[Security/Extension] Extension Authentication Backend

[RyanL1997]

Description

Extension Authentication Backend

Some additional modification in JWTVendor:

• Add the support of BWCMode
  - BWCMode On: plain text roles will be passed into claim. It also accepts nonString configuration, but it will be converted into string as the payload. The roles information will be stored in the payload field dec_r (standing for decrypted roles).
  - BMCMode Off: encrypted roles will be passed into claim, and it will always be a encrypted string. The roles information will be stored in the payload field enc_r (standing for encrypted roles).

Category

New feature

Issues Resolved

• Resolve: [Extension] Authentication Backend for JIT Token Validation #2619

Check List

• New functionality includes testing
• New functionality has been documented
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[RyanL1997]

Testing is WIP

[stephen-crawford]

Hi Ryan, I wanted to give you some early feedback. Overall, looks like you are on the right track! Great tests!

[stephen-crawford]

Should this throw an error instead of just logging?

[RyanL1997]

Yes this will directly skip the else and throw another error log L110 with an runtime exception at L112. The way we want to do it in this way is because we would like to log some specific information for user to know the reason of failure creation of this token. There are also some try/catch statements in the else for checking encryption algorithms, and each of them will work in the same pattern. It will tell the user why they failed first by these error logs and give them the runtime exception at L112.

[DarshitChanpura]

shouldn't this require a throw statement in order for catch block at L110 to catch it?

[stephen-crawford]

Is this a standard-practice implementation? It may be possible to replace this with .match(BEARER) or something similar

[RyanL1997]

Yes, I think it is also work with the .match(). I was following the same expression in HTTPJwtAuthenticator.java (L170). So, maybe we should just leave like this.

[stephen-crawford]

Do we want to allow this conversion or should we just throw an error? What do you think?

[RyanL1997]

In the original HTTPJwtAuthenticator.java it does the same thing. I believe the reason to do this is that we also need to handle the case for some nonString claims. In the test case, we also have a test case called testNonStringClaim. I think this scenario only happens when the user enable the BWCMode, which means instead of passing encoded roles, plain text roles will be passed into the claim. For the disabling of BMCMode, the role will always be encrypted, so that it will always be an encrypted string.

[cwperks]

The message here shouldn't contain the word roles. Can you update the message to Expected type String for subject...

This should be updated in the HTTPJwtAuthenticator as well. There is overlap between here and the HTTPJwtAuthenticator class. Maybe we can create a utility class with static methods or maybe add these to an abstract class that both inherit from?

[RyanL1997]

I'm working on the fix of just 1 test case: testRolesArray and some code hygiene issue. Other than that this PR is ready for review.

[RyanL1997]

The CI will be fixed after the merge of #2715

[cwperks]

Does this mean that this takes anything passed in the Authorization header as long as it doesn't contain the word Basic anywhere? In the next block if it contains the word bearer it will strip that out and trim the rest?

[cwperks]

Hey @RyanL1997 , I took a first pass at this and like the progress and tests that have been added here. Do you have any thoughts about configuration of this backend? Is this a special backend that if extensions is enabled then is evaluated before the list of auth domains in the authc section of the security plugin config.yml file? Have you been able to install the security plugin with this change into an opensearch node and show the change in use?

[cwperks]

The null check isn't needed here or in the else if, this will never be null.

[cwperks]

The message here shouldn't contain the word roles. Can you update the message to Expected type String for subject...

This should be updated in the HTTPJwtAuthenticator as well. There is overlap between here and the HTTPJwtAuthenticator class. Maybe we can create a utility class with static methods or maybe add these to an abstract class that both inherit from?

[stephen-crawford]

Hi @RyanL1997, things look very good here. I would be happy to approve if you are able to address Craig's comments. I think he covered everything.

[codecov-commenter]

Codecov Report

Merging #2672 (895b8d0) into feature/extensions (d4e5f1f) will increase coverage by 0.02%.
The diff coverage is 70.27%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@                   Coverage Diff                    @@
##             feature/extensions    #2672      +/-   ##
========================================================
+ Coverage                 61.57%   61.59%   +0.02%     
- Complexity                 3409     3426      +17     
========================================================
  Files                       274      275       +1     
  Lines                     18836    18971     +135     
  Branches                   3293     3309      +16     
========================================================
+ Hits                      11598    11686      +88     
- Misses                     5638     5674      +36     
- Partials                   1600     1611      +11     

Impacted Files	Coverage Δ
.../org/opensearch/security/auth/BackendRegistry.java	63.17% <ø> (ø)
.../security/http/HTTPOnBehalfOfJwtAuthenticator.java	69.44% <69.44%> (ø)
.../opensearch/security/OpenSearchSecurityPlugin.java	80.35% <100.00%> (+0.07%)	⬆️
...g/opensearch/security/authtoken/jwt/JwtVendor.java	65.82% <100.00%> (ø)

... and 7 files with indirect coverage changes

[peternied]

What are your thoughts on this package, com.amazon doesn't make much sense to me. I'd suggest a move this under a org.opensearch.security.auth package or child package.

[RyanL1997]

Fixed. That makes sense to me too. I was doing this because I was following the class of HTTPJwtAuthenticator.

[peternied]

Why does this authenticating backend check for BASIC auth headers, shouldn't it only support bearer auth?

[RyanL1997]

That was the question I raised up during one of our stand-up meetings. I assume that the HTTPJwtAuthenticator were doing this could be due to leniency in handling to support non-standard header formats that do not include the “Bearer” keyword explicitly. However, for our case, we are require the keyword "Bearer" for our auth header. I will fix this in the next commit.

[RyanL1997]

Fixed!

[peternied]

Is their a specific action that needs the permission? The InternalAuthenticationBackend doesn't seem to need this PrivilegedAction.

[peternied]

Take a look at these classes from @scrawfor99 's PR, they do a good job isolating the specific functionality from the overall context. While you might not need extra classes here, I think the parsing extracting logic would be cleaner

• RestTokenExtractor
• BasicAuthToken

[DarshitChanpura]

+1 on moving extraction logic to an isolated class. Could be done in a follow-up PR tho

[RyanL1997]

@peternied and @DarshitChanpura , Thanks for the information and suggestions. Yes I think this is a good idea. We can do that as the follow-up PR for linking this backend with the configuration setup.

[peternied]

While extensions will use this functionality. This class is a delegate authenticating backend, lets remove the name extension and reframe to its purpose, what do you think of this?

[RyanL1997]

Good point~ Have already changed it to HTTPOnBehalfOfJwtAuthenticator, what do you think about this one?

[DarshitChanpura]

+1 on moving extraction logic to an isolated class. Could be done in a follow-up PR tho

[DarshitChanpura]

Suggested change

	String jwtToken = request.header(HttpHeaders.AUTHORIZATION);
	if (jwtToken != null && BASIC.matcher(jwtToken).matches()) {
	jwtToken = null;
	}
	if (jwtToken == null || jwtToken.length() == 0) {
	if(log.isDebugEnabled()) {
	log.debug("No JWT token found in '{}' header", HttpHeaders.AUTHORIZATION);
	}
	return null;
	}
	String authHeader = request.header(HttpHeaders.AUTHORIZATION);
	if (authHeader == null || authHeader.length() == 0 || BASIC.matcher(authHeader).matches()) {
	if(log.isDebugEnabled()) {
	log.debug("No JWT token found in '{}' header", HttpHeaders.AUTHORIZATION);
	}
	return null;
	}

[stephen-crawford]

Looks good Ryan! Great job :)

[stephen-crawford]

I am not sure what "er" means. I know there are some conflicting opinions, but I am of the mind that we lose meaning at a certain point of abbreviating things. I am guessing this stands for "____ roles"? Perhaps we can make this a more meaningful key?

[DarshitChanpura]

i think er stands for encrypted roles.

[cwperks]

Thank you @RyanL1997! I left 2 comments that are pretty minor. Once addressed I will approve this PR and we can merge it into the feature branch.

[cwperks]

Thank you @RyanL1997! I left 2 comments that are pretty minor. Once addressed I will approve this PR and we can merge it into the feature branch.

[DarshitChanpura]

Nicely done!

[RyanL1997]

Merging, and the CI is failing due to the new introduced term check for 'extension' at previous PR: Extensions config for JWT signing/encryption key (#2671). I have created a follow-up issue at: #2760