Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP - Expose attributes to script attribute repositories via thread local #4696

Closed
wants to merge 7 commits into from

Conversation

@hdeadman
Copy link
Member

hdeadman commented Feb 14, 2020

This stores an "in progress" principal in a thread local so it (the attributes in it) can be accessed from script attribute repositories. It includes attributes gathered during authentication and accumulates attributes from the attribute repositories as they are queried in order.

Accessing the attributes from a script would look like this:

def Map<String, List<Object>> run(final Object... args) {
    def uid = args[0]
    def logger = args[1]
    def principal = org.apereo.cas.web.support.WebUtils.getInProgressPrincipal()
    def attributes = principal.getAttributes()
    logger.warn("Attributes from thread local! {}", attributes)

I have a use-case where x509 and LDAP auth are both supported, with some x509 users in LDAP and some not. During X509 auth LDAP is queried via user principal from cert SAN and additional attributes are retrieved (if user is in LDAP). Another attribute repository (groovy script) needs access to the subject DN (another X509 attribute) in order to make an external request for more attributes.

I updated the two principal resolvers that add attributes from the credentials (x509 and WSFED) and I saw during testing that some attributes gathered from LDAP during authentication (for use by PM module) were available in a script attribute repository.

This is WIP pending feedback and then I can add tests and documentation.

hdeadman and others added 4 commits Feb 14, 2020
Copy link
Member

mmoayyed left a comment

I think we need to review and discuss the use case in more detail to find an alternative. The patch here, while likely adequate to address the need feels a bit too brittle.

/**
* Bind Principal to ThreadLocal for principal that has been authenticated but for which attributes have not yet been retrieved.
* This can make attributes from the authentication event (e.g. X509, LDAP, WSFED) available to scripted attribute repositories.
* If a principal is already bound, combine the attributes with new attributes overriding existing.

This comment has been minimized.

Copy link
@mmoayyed

mmoayyed Feb 16, 2020

Member

Nitpicking here: to scripted attribute repositories is a little too specific. I might suggest that the description here be generalized a bit more; something like "to consumer components that may not have immediate access to the resolved principal, such as ...*

val inProgressPrincipal = IN_PROGRESS_PRINCIPAL.get();
if (inProgressPrincipal != null) {
inProgressPrincipal.getAttributes().putAll(principal.getAttributes());
} else {
IN_PROGRESS_PRINCIPAL.set(principal);
}
Comment on lines +47 to +52

This comment has been minimized.

Copy link
@mmoayyed

mmoayyed Feb 16, 2020

Member

This does not seem safe; this is not just binding a principal but also modifying it. If we just want to expose a principal that is already resolved, sure, as that would be basically get and set ops. But this changes things that make it very difficult to troubleshoot or even maintain, specially the manual copying of the attributes. For example:

  1. If the Principal interface is ever modified to include additional fields/flags, we'd have to track this area to make sure we are also copying more stuff...or realize why we don't or shouldn't have to.
  2. It appears as though you are not really binding the principal, but you do so only to fetch attributes that are attached to it. Semantically, this feels strange. i.e., what is stopping an API call from binding a principal that is different from what's already tracked and bound, maybe even with a different identifier?

This comment has been minimized.

Copy link
@hdeadman

hdeadman Feb 16, 2020

Author Member

Agreed that I shouldn't be using the Principal object, it was convenient b/c it had the attributes map I wanted, and I was undecided about whether to save the attributes map directly or introduce a new object/interface that only contained the attributes map. The attributes map gets passed as-is to attribute release policies where it is used in scripts so it sort of an API object on its own...

I am trying to accumulate attributes including the ones from the authentication and make them available to subsequent attribute repositories and using a thread-local avoids having to add more APIs and complexity to person-directory where many of the attribute repositories would probably not benefit from the changes.

/**
* Gets in progress principal for use by scripted attribute repositories.
* This method just allows scripts to use WebUtils instead of the longer class that is more likely to change.
*
* @return the in progress principal
*/
public static Principal getInProgressPrincipal() {
return AuthenticationCredentialsThreadLocalBinder.getInProgressPrincipal();
}

Comment on lines +856 to +865

This comment has been minimized.

Copy link
@mmoayyed

mmoayyed Feb 16, 2020

Member

I am not sure this belongs here; AuthenticationCredentialsThreadLocalBinder.getInProgressPrincipal() should be enough, and wrapping it here is not appropriate because it's not a web/webflow utility.

@mmoayyed

This comment has been minimized.

Copy link
Member

mmoayyed commented Feb 16, 2020

Let me clarify and confirm a few things:

You have an X509 resolver that fetches a principal from a cert attribute, and uses this attribute to query LDAP to collect more attributes to build the final principal. So once this step is complete, you have a resolved Principal that contains some stuff from X509, and may or may not contain additional attributes from LDAP. Is that correct?

Then, you have a Groovy attribute repository(?) that needs access to this already-resolved principal to make a separate call to some other resource to fetch attributes and more, right? If so, why do you need to do this in a Groovy script outside the already-available and processed principal resolution engine, and do this strange cross-walk?

Why not, simply:

  1. Run X509 to extract info/attributes
  2. Run LDAP next to fetch more attributes
  3. Call some other API to yet ask for more.

Do this all inside your own resolver and inject it into CAS as custom code. The solution that you have here seems strangely complicated for something that might be customized and injected into CAS in form of a simple resolver with code re-use from existing components.

@hdeadman

This comment has been minimized.

Copy link
Member Author

hdeadman commented Feb 16, 2020

Thanks for the review and the comments. I agree this needs work but I got it working and wanted feedback before investing more time in it.

Let me clarify and confirm a few things:

You have an X509 resolver that fetches a principal from a cert attribute, and uses this attribute to query LDAP to collect more attributes to build the final principal. So once this step is complete, you have a resolved Principal that contains some stuff from X509, and may or may not contain additional attributes from LDAP. Is that correct?

Then, you have a Groovy attribute repository(?) that needs access to this already-resolved principal to make a separate call to some other resource to fetch attributes and more, right? If so, why do you need to do this in a Groovy script outside the already-available and processed principal resolution engine, and do this strange cross-walk?

I am using the SUBJECT_ALT_NAME x509 authentication b/c that is the link to the user in LDAP but I also need to query attributes (using groovy attribute repository) from an external service that requires the subjectDn and the issuerDn which are both attributes extracted from the certificate. I have a java client that the groovy script calls since the service requires x509 client auth and the results need to be processed into CAS attributes. Theoretically (if http request is on thread local) I could re-parse the certificate and re-extract the attributes but I would rather not have to do that if the attributes could be made available to groovy since they were already parsed out by CAS.

Why not, simply:

  1. Run X509 to extract info/attributes
  2. Run LDAP next to fetch more attributes
  3. Call some other API to yet ask for more.

Do this all inside your own resolver and inject it into CAS as custom code. The solution that you have here seems strangely complicated for something that might be customized and injected into CAS in form of a simple resolver with code re-use from existing components.

To date I have been able to do everything I need to do (for three different CAS deployments: dev and two different production environments) without custom Java code other than the client code for external services but that Java code is not aware of CAS, it is just bundled with CAS so it can be called from groovy. Each deployment has different service definitions and spring profiles allow for different properties and allow for different selections of attribute repositories but they all run using the same CAS image and no custom CAS code (because CAS has so many configuration options it hasn't been necessary). I was getting away with using x509 SUBJECT_ALT_NAME in dev and x509 SUBJECT_DN in production environments which meant that I was able to use SUBJECT_DN in the groovy script to call the external service. Now I need to support some internal tools outside of dev where LDAP will have to be queried (by either SAN or EMAIL depending on environment) and I also need to support a new external service that needs subject DN and issuer DN as arguments for attribute retrieval.

If you don't think this can be cleaned up and made less brittle/more maintainable then I can go the custom resolver route, I just thought that what is useful for one person might be useful for others. I realize x509 is only used in a few niche areas (mostly govt) but I would imagine being able to manufacture or retrieve attributes using the attributes gathered from previous attribute repositories would be generally useful but people have survived without it until now so maybe I am wrong.

@mmoayyed

This comment has been minimized.

Copy link
Member

mmoayyed commented Feb 18, 2020

Thanks for the notes. Most helpful.

If you don't think this can be cleaned up and made less brittle/more maintainable then I can go the custom resolver route, I just thought that what is useful for one person might be useful for others. I realize x509 is only used in a few niche areas (mostly govt) but I would imagine being able to manufacture or retrieve attributes using the attributes gathered from previous attribute repositories would be generally useful but people have survived without it until now so maybe I am wrong.

Definitely; I fully agree. Exposing what's ready resolved to downstream components such as attribute repositories is quite legitimate and an attractive use case and something we ought to support, and I personally would very much like for CAS to handle it without having you write custom code for sure. That said, the way we are going about it should be simplified and I have a proposal/suggestion that might get us the best of both worlds:

This is the sort of use case that would require API re-work and much of it likely needs to happen inside PersonDirectory. At this point, we are using a ThreadLocal instance as a bandaid and a bridge to store what's resolved and expose it later to attribute repositories, etc. I propose that it would be much better, semantically, if the attribute repository and family could directly receive and operate on what's resolved via known APIs and parameter passing, etc vs holding thread-local contexts for later access.

In other words, instead of:

1- Resolve the principal from authn
2- Hold it in a context
3- Call the attribute repository to fetch/process it
4- Change the object in-flight and/or update it to finalize

I submit that we are much better off to:

1- Resolve the principal from authn
2- Pass it to the attribute repository directory via an API mechanism used by CAS and owned/designed/exposed by Person Directory
3- Collect the final person from the repository and rebuild the Principal as a whole.

Steps 1 and 3 already are pretty much covered. We should just focus on step 2.

This allows for:

  • No thread-local; which does come with minor performance costs.
  • No in-flight updates to change thread state.
  • No exposing the principal to other components. You don't want others to call AuthenticationCredentialsThreadLocalBinder.getInProgressPrincipal()
  • Exposing this to Person Directory makes it useful for other consuming projects such as uPortal, etc.
  • Keeps the data-transfer encapsulated via known manageable APIs.

Certainly it's more work, but I'd argue that it's likely easier to maintain and reason about and now that PersonDirectory is gearing up for a v2 release, it's perfect timing to maybe rethink APIs and allow for this sort of flexibility there.

I hope you find this agreeable, and if so, we should probably start by a blueprint proposal of an API for Person Directory to see what we need to change at a minimum to allow for this use case there. Since you are the use-case owner, you're in a perfect position to start the work and back it up by a real deployment environment. You're most welcome to start with a general outline, or if you prefer, I can try to put something together for person directory to make it easier for you to test it from the CAS perspective.

@hdeadman

This comment has been minimized.

Copy link
Member Author

hdeadman commented Feb 18, 2020

I agree that adding it to the person-directory API is probably the right way to go. In the short term so I can get a release out I may follow your suggestion of temporarily making a custom principal resolver bean, since for my specific use-case I really just need to override the x509 principal resolver bean and I should be able to create and cleanup a thread local just by overriding the retrievePersonAttributes method.

I can help out with the person-directory changes but I would prefer if you proposed the API change and I could work on implementing it. When I looked at it before it almost seemed like the API might be there since they let you call getPeople with a Map<String, Object> query but that is called a "query" and IIRC they currently just pick one attribute out of the map and use that for finding the person, and they don't expose the full "query" to the scripted attribute repositories. Also, CAS isn't using those versions currently but maybe the fix could be to use those methods and let that be the attributes map with the principal thrown in there and person-directory configured to pull out the principal but also make person-directory pass the full "query" map to the scripted attr. repositories as a third argument (or have me implement a custom IPersonAttributeDao for my external service client). I didn't really understand the intent of that version of the method and I wonder if it was an attempt to address my use-case (needing multiple attributes to get more attributes about the user).

There are also some deprecated methods in IPersonAttributeDao and maybe we should remove those as part of 2.0 (they were deprecated in 2008).

@mmoayyed

This comment has been minimized.

Copy link
Member

mmoayyed commented Feb 19, 2020

Sounds good, sure. I'll try to work on a proposal, with some additional adjustments, some time next week for you to review.

@hdeadman hdeadman closed this Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.