Implement the CAS server as a Trust Anchor (2/2)#8862
Conversation
mmoayyed
left a comment
There was a problem hiding this comment.
I will come back with more intelligent comments later, but just wanted to give a quick heads up here that modeling entities as CAS services is not the right move in my opinion. While I don't have good answers yet, it would be likely best to externalize those as a JSON resource that can be modeled and read freely.
| * @return whether the role is TA or intermediate. | ||
| */ | ||
| public static boolean isTaOrIntermediate(final OidcFederationRole role) { | ||
| return role == TRUST_ANCHOR || role == INTERMEDIATE; |
There was a problem hiding this comment.
I don't think this reads well, but it might be me. Rather than using a static method, it might be better to do role.isTrustAnchorOrIntermediate() directly on the role object itself.
There was a problem hiding this comment.
Yes, this is too complicated and awkward. I will change that
| @EqualsAndHashCode(callSuper = true) | ||
| @NoArgsConstructor | ||
| @Accessors(chain = true) | ||
| public class OidcFederationEntityService extends BaseWebBasedRegisteredService { |
There was a problem hiding this comment.
-1 here.
I don't think this is the right move. Among other things, this can never be supported by the Palantir admin module, or it would be very very very difficult to do so.
There was a problem hiding this comment.
I'm not sure why this would be a problem in Palantir as this tool especially edits any JSON...
There was a problem hiding this comment.
No that is not always the case. Palantir's main function allows for a wizard-based approach for adding and managing services, which requires proper models and fields in all service types. The JSON editor is a stop-gap and will soon go away. Furthermore, the fact that the new service type is not serializable, cannot be supported by most service registries, has no locator concept and thus entries cannot be located like other types, does not model the fields it presents properly, is not really an application (as I understand though I could be wrong here) one would want to model as a service anyway, and is almost identical to the OIDC registered service and duplicates many fields, (and likely a few other reasons!) make this proposal difficult to accept and maintain.
I think this needs to work similar to how SAML2 metadata in large (aggregate) XML files are managed. There, you have the entities defined in an external resource, like an file using a data representation format like XML. You need not model those as a service, and not everything in that file is something one would want to use anyway. Likewise, I think here you need to move the collection of entities out of service registry and keep them outside CAS as a file, etc or even a set of files in a directory, etc. This way, you won't be tied to the service model, and can change that freely, do not have to support Palantir, or worry about serialization and modeling issues, etc. This data most likely will be fed to CAS at some point and will be imported from elsewhere anyway.
If you really really really want to use the service registry model, then you have to reuse the OIDC registered service, and model the missing fields there as necessary (which sounds like it might be just the federation-keys element. In a TA deployment, the registry would only be housing such entities and as I understand, it's not running as a normal CAS server anyway. (You also have to support any new fields you add in Palantir). You also have make a concession that a TA can only support openid_relying_party since it's modeled as an OIDC Registered Service. As you likely can predict, new problems continue to show up with this route, and I would strongly advise against this approach. Keeping these outside the registry seems like a better fit to me.
There was a problem hiding this comment.
I could have done that: keeping data outside the registry; I have the feeling this would not have been more or less work, but let me share my reasoning.
Conceptually, I wanted to reuse the services for the TA support.
For a CAS server acting as an OIDC server (OP), each client (RP) must be defined as a service. Services are the components integrated with the CAS server and the feature in that case is the OIDC protocol.
For a CAS server acting as a trust anchor, the entities integrated with the CAS server are the subordinates and the feature here is the OIDC federation protocol.
To sum up:
(CAS server) OP <- OIDC authentication -> RP (service)
(CAS server) TA <- OIDC federation -> subordinate (service).
Yes, you're right, the current state of the OidcFederationEntityService is quite limited and may not be sufficient, but we have very strong similarities between OIDC services and OIDC subordinates. Despite the current basic implementation in TA, subordinates could be queried, listed, added, removed, etc. like any CAS services.
I would not drive my design decisions based on the amount of work.
I think it makes sens to have subordinates as services, theoretically speaking.
Though, as practically this is the first and simple implementation of CAS as a TA, I have no problem to say that the only supported service registry is the JSON one and edition is not supported by Palantir. And maybe this will remain this way for ever.
If by chance, that support (TA) gains popularity and a lot of adoption and requests, we will extend its support scope.
Eventually, as the Chairmain, it's your call.
There was a problem hiding this comment.
Thank you very much for the notes; I now have a better understanding of how this is supposed to work.
Based on your notes above, it seems to be that a CAS server that is running as a TA (whose only job likely is to be a TA and nothing else) should be able to manage "subordinate" services as OidcRegisteredService entries that can be managed via the registry. I don't then really see why there needs to be a separate OidcFederationEntityService; these subordinate entries are applications/services and we already have a model that represents an application for OIDC. The duplication of data models does not make sense to me.
The current choices here present the following problems:
-
As discussed there is no reason for
OidcFederationEntityServiceto exist, since it in almost every way duplicatesOidcRegisteredService. Conceptually, it represents an application and there is already a model for that. -
OidcFederationEntityServiceappears not serializable and cannot be, in its current state, supported by almost all service registries. -
OidcFederationEntityServiceis inconsistent design-wise; it declaresJsonNodefields that simply allow it to carry a blob of JSON. This contradicts every other model that we have where fields are properly defined. I don't see a technical reason for this, other than "simplicity". To me, this is more confusing. -
Using
OidcFederationEntityServicemeans you automatically lose support for the likes of Palantir module. That is of course perfectly acceptable and we do not have to support that immediately. However the choices here present a few more subtle problems:
- the current state of
OidcFederationEntityServicemakes it very very very hard to support it in Palantir at some point in the future, and - when the time comes, it seems like we might have to redo
OidcFederationEntityServicethis anyway to properly model fields and entries, and - at least for me, it's difficult for me to justify or imagine why a deployment would want to use this and pay for its evolution in this form.
You also have to consider that that because OidcFederationEntityService now exists, you have to:
- document it separately (as you did) vs "This is just another OIDC service like anything else"
- maintain, test and manage it separately vs "This is just another OIDC service like anything else"
- teach users and deployers about its configuration separately
- help them switch to a different learning model vs "This is just another OIDC service like anything else".
I find this to be a lot more work, to be honest. This is exactly why, in my head at least, it would be too difficult to see why anyone would want to adopt this and pay for us to make it better or support it in Palantir, etc.
While I have not done the work myself, at first glance it looks as though using OidcRegisteredService instead of OidcFederationEntityService should be doable. 99% the fields are exactly the same conceptually, and you can add what is missing (and only document the missing bit). The only serious work that remains is translating OidcRegisteredService to a JSON format that is appropriate for TAs.
Going forward, I think that makes life much easier. You can always add OidcFederationEntityService later if necessary, but you cannot easily remove it or rework its internals, etc once it's there (specially after time has passed and you have a few deployments on it).
There was a problem hiding this comment.
Let's narrow the scope down to openid_relying_party only which matches the function of OIDC Registered Service. We will document that this is the only type we supported as a TA, and we then can wait for feedback, adoption and demand. For the time being, your render of OidcRegisteredService to JSON should only support openid_relying_party or in general, any type that can represent a relying party / application.
There was a problem hiding this comment.
Yes, we can limit the scope of this feature (I have no problem to only support the JSON services registry for example).
Though, we cannot only support RPs. That would make no sense.
The idea of the federation is to allow RPs to find OPs without needing to be registered, merely through a trust third-party, the trust anchor.
Take a look at: https://www.pac4j.org/blog/openid_federation_with_pac4j_and_connect2id.html This is the environment I used for integration tests. You have a whole federation consisting of a RP (pac4j), an OP (Connect2id) and a TA (simulated). With the latest v8.0.0-SNAPSHOT of the CAS server, I can locally use the CAS server as the trust anchor instead, but for that, I need to define both the RP and the OP as its subordinates.
There was a problem hiding this comment.
Thanks for the reference; I understand the scope.
The Service Registry's purpose and design is intended to handle Applications as you well know. Relying Parties, Service Providers, etc. It would make sense only to use this model for anything that deals with openid_relying_party. If we need to support OPs, then this is not the right fit. It becomes an exception: why are we treating OPs and identity providers as "services" and managing them in the service registry?
So I don't think it is a good idea to use the service registry here then if you need to support more than openid_relying_party. The management aspect of federation entities must be done outside the service registry. You can build a new abstraction that can hold all types, i.e. FederationRepository, that would likely be useful and reusable for TA and non-TA deployments, or simply put everything into a JSON file (or many) and load the data from there, providing CRUD support.
I see no other maintainable options, long-term.
There was a problem hiding this comment.
Yes, the service registry holds applications as this is the component integrated with the authentication server whatever the protocol.
When it comes to federation, the integrated components are RP, OP, intermediate, ... so it could have made sense to have them in the services registry.
But you're right, it depends on what definition we want to put on the services registry.
Any way, I will handle subordinates outside the service registry, but I fear that I might be somehow reinventing the JSON registry for my federation entities.
There was a problem hiding this comment.
The PR has been updated to use a custom repository.
|
@mmoayyed No hurry. No pressure. But I wanted to remind you about this PR. |
This is for task 6a from NLnet: Implement the CAS server as a Trust Anchor.
And this is the second and last task of 2.
This PR mainly adds:
org.apereo.cas.oidc.federation.service.OidcFederationEntityServiceto define subordinates with their metadata and federation keys/fetchendpoint via theOidcTrustAnchorFetchEndpointController.For simplicity and security, the metadata and federation keys must be defined in the service definition.
The Puppeteer tests have been updated and the
OidcTrustAnchorFetchEndpointControllerTestsunit test confirms the expectations.The documentation has been updated as well.
Once again, there is no end-to-end integration test as the CAS server is not a federated OP yet (but I will add that at the end of task 6). Though, I restarted from:
https://www.pac4j.org/blog/openid_federation_with_pac4j_and_connect2id.htmland used the CAS server as the trust anchor (instead of the simulated one) and it perfectly works (I will prepare some article on this).