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

[Feature/Identity] Strategy for Delegated Authority using Tokens #4826

Merged
merged 19 commits into from
Nov 9, 2022

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Oct 18, 2022

Description

This PR contains documentation for token usage in OpenSearch security today and how the usage can be extended to provide delegated authority for extensions running work asynchronously. This PR also contains sample code for token creation and verification.

This PR adds a docs/ folder in the new sandbox lib which can be a good place to store documents related to feature/identity.

Issues Resolved

Related to: #3850

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

sandbox/libs/authn/docs/jwt.md Outdated Show resolved Hide resolved
it clearer to understand what privileges are in use when the job is executing and gives an administrator the ability to fine tune the security model for their cluster.
In addition to offering better fine tuning for security, it also gives administrators the power to revoke privileges to prevent an errant job from executing.

In order to delegate authority for a background task or job, I propose to create a token vending service to produce tokens that confer access to the cluster and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing scenario, when a request comes into the cluster, it might only execute on the node it arrives it, or it might be distributed to another one nodes or even all of the nodes in the cluster.

Retransmitted authentication information such as username/password would be troublesome, as would be additional requests to a SAML or OIDC provider that could be negatively impacted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great use-case to point out in this document. Adding.

sandbox/libs/authn/docs/jwt.md Outdated Show resolved Hide resolved

`curl -XGET -H "Authorization: Bearer ${ACCESS_TOKEN}" http://localhost:9200`

This is often referred to as Bearer Authentication
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


package org.opensearch.authn.jwt;

import org.apache.cxf.jaxrs.json.basic.JsonMapObjectReaderWriter;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for pulling in CXF over say https://github.com/auth0/java-jwt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main implementation uses jjwt which is also used by the security plug-in. I will have to take a deeper look at this other library to understand the trade-offs.

I will write a section in the markdown document on the libraries in use and what each is used for to understand the need for these dependencies.

implementation 'io.jsonwebtoken:jjwt-api:0.10.8'
implementation('org.apache.cxf:cxf-rt-rs-security-jose:3.4.5') {
  exclude(group: 'jakarta.activation', module: 'jakarta.activation-api')
}

runtimeOnly 'org.apache.cxf:cxf-core:3.4.5'
implementation 'org.apache.cxf:cxf-rt-rs-json-basic:3.4.5'
runtimeOnly 'org.apache.cxf:cxf-rt-security:3.4.5'

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:


public class BadCredentialsException extends Exception {

private static final long serialVersionUID = 9092575587366580869L;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this representing core? It seems like it is a dead variable

Copy link
Member Author

@cwperks cwperks Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See discussion on SO about serialVersionUID: https://stackoverflow.com/a/285809

This initial implementation was inspired by JWTs within the security plugin and this class is a copy and admittedly this should changed since the fully-qualified class name has changed.

Doing a quick search of the security repo: https://github.com/opensearch-project/security/search?p=1&q=serialVersionUID it looks like exceptions related to auth have this value set, but I'm not exactly sure why.

JsonWebKey jwk = new JsonWebKey();

jwk.setKeyType(KeyType.OCTET);
jwk.setAlgorithm("HS512");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why all the way up to 512? 256 will be fine I would imagine, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the same defaults already in use today: https://github.com/opensearch-project/security/blob/f431ec2201e1466b7c12528347a1f54cf64387c9/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java#L262-L269

On a future PR Feature/Identity needs to have a way of configuring settings which can override the defaults.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

Gradle Check (Jenkins) Run Completed with:

Creation of an asynchronous job or task should require a Refresh Token on creation which is associated with a subject and let's the job run with the subject's permissions. When the task
starts execution it will use the refresh token to obtain an access token which will allow the task to interact with the cluster as the subject who the token was created for.

Refresh tokens will be associated with a subject (and an extension?) and both the subject and administrators (or subjects with requisite permissions to revoke tokens of others) will be able to revoke access to the tokens. Issuing a token for oneself or on behalf of others will be another set of permissions that can be assigned.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is where our principal on the Subject might need to transition from a singleton to collection, e.g.:

List<Principal> = List.of(
   new StringPrincipal("InternalUser:Danny),
   new StringPrincipal("Extension:SudoRm")
);

So when doing subject.isAllowed("opensearch:admin/wreck/thisCluster"), we could verify that both the internal user Danny has the permissions AND that extension SudoRm has the permissions too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To accomplish extension specific tokens we will need to manage a secret for each extension to sign the token with.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets try to sync on internal vs user actions section, other comments are optional

sandbox/libs/authn/docs/jwt.md Outdated Show resolved Hide resolved
…ed authority

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any outstanding comments, I know you might be spending some time on this - ready whenever you'd like another review or feel free to flip from draft and we can look at merging

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Gradle Check (Jenkins) Run Completed with:

@cwperks cwperks marked this pull request as ready for review November 9, 2022 18:53
@cwperks cwperks requested review from a team and reta as code owners November 9, 2022 18:53
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwperks The image you added doesn't render in this diff, maybe you need to re-add it. but I'm going to merge this anyways based on all the build components you were fighting with

@peternied peternied merged commit d3b60b5 into opensearch-project:feature/identity Nov 9, 2022
@cwperks
Copy link
Member Author

cwperks commented Nov 9, 2022

@peternied In my next PR with tokens around internal actions I'll see if I can get the image to render. Its a basic image of a cluster, client and extensions without any more info but I plan to add more to it.

authn_with_tokens_draft

@peternied
Copy link
Member

The image you just posted works, shrug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants