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

Adding audience support #13

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@djw8605
Contributor

djw8605 commented Sep 11, 2018

No description provided.

@djw8605 djw8605 requested a review from bbockelm Sep 11, 2018

@djw8605 djw8605 removed the do-not-merge label Sep 11, 2018

@djw8605

This comment has been minimized.

Show comment
Hide comment
@djw8605

djw8605 Sep 11, 2018

Contributor

This fixes #12

Contributor

djw8605 commented Sep 11, 2018

This fixes #12

@bbockelm

This comment has been minimized.

Show comment
Hide comment
@bbockelm

bbockelm Sep 11, 2018

Contributor

Please add the ability to do multiple audiences (i.e., accepts "T2_US_Nebraska" or "https://xrootd-local.unl.edu").

How did you test it? I assume three tests are needed:

  • How we handle the case where no audience is stated (outcome: accept)
  • Case where audience matches (outcome: accept)
  • Case where audience does not match (outcome: reject)
Contributor

bbockelm commented Sep 11, 2018

Please add the ability to do multiple audiences (i.e., accepts "T2_US_Nebraska" or "https://xrootd-local.unl.edu").

How did you test it? I assume three tests are needed:

  • How we handle the case where no audience is stated (outcome: accept)
  • Case where audience matches (outcome: accept)
  • Case where audience does not match (outcome: reject)
@jjg-123

This comment has been minimized.

Show comment
Hide comment
@jjg-123

jjg-123 Sep 11, 2018

jjg-123 commented Sep 11, 2018

@djw8605

This comment has been minimized.

Show comment
Hide comment
@djw8605

djw8605 Sep 12, 2018

Contributor

The JWT RFC explicitly says that aud is optional, I am very hesitant to deviate from that.

Contributor

djw8605 commented Sep 12, 2018

The JWT RFC explicitly says that aud is optional, I am very hesitant to deviate from that.

@djw8605

This comment has been minimized.

Show comment
Hide comment
@djw8605

djw8605 Sep 12, 2018

Contributor

How we handle the case where no audience is stated (outcome: accept)

Yup

Case where audience matches (outcome: accept)

Yup

Case where audience does not match (outcome: reject)

Yup. This also includes the case where the aud is not specified in the token, but is on the server. And the opposite.

Contributor

djw8605 commented Sep 12, 2018

How we handle the case where no audience is stated (outcome: accept)

Yup

Case where audience matches (outcome: accept)

Yup

Case where audience does not match (outcome: reject)

Yup. This also includes the case where the aud is not specified in the token, but is on the server. And the opposite.

@jjg-123

This comment has been minimized.

Show comment
Hide comment
@jjg-123

jjg-123 Sep 12, 2018

jjg-123 commented Sep 12, 2018

@bbockelm

This comment has been minimized.

Show comment
Hide comment
@bbockelm

bbockelm Sep 12, 2018

Contributor

Hi,

Automatically rejecting would be a fairly significant break of the semantics for existing use cases. We have a mechanism for this, however, that we've previously discussed at length - versioning.

If we really want to always include aud, then we can bump the version number and then change the rules accordingly. Each implementation is supposed to know the minimum and maximum version number it can parse.

I'm fine with making aud mandatory for SciTokens, but not without using the existing mechanisms for doing such a change.

Brian

Contributor

bbockelm commented Sep 12, 2018

Hi,

Automatically rejecting would be a fairly significant break of the semantics for existing use cases. We have a mechanism for this, however, that we've previously discussed at length - versioning.

If we really want to always include aud, then we can bump the version number and then change the rules accordingly. Each implementation is supposed to know the minimum and maximum version number it can parse.

I'm fine with making aud mandatory for SciTokens, but not without using the existing mechanisms for doing such a change.

Brian

@jbasney

This comment has been minimized.

Show comment
Hide comment
@jbasney

jbasney Sep 12, 2018

Member

draft-ietf-oauth-jwt-bcp-03 contains some relevant guidance:

If the same issuer can issue JWTs that are intended for use by more
than one relying party or application, the JWT MUST contain an "aud"
(audience) claim that can be used to determine whether the JWT is
being used by an intended party or was substituted by an attacker at
an unintended party. Furthermore, the relying party or application
MUST validate the audience value and if the audience value is not
present or not associated with the recipient, it MUST reject the JWT.

Just a draft at this point but it may be a useful reference on this and other JWT best practices.

I agree with Brian's comment about versioning. We don't want to break what's currently working for OSG.

Member

jbasney commented Sep 12, 2018

draft-ietf-oauth-jwt-bcp-03 contains some relevant guidance:

If the same issuer can issue JWTs that are intended for use by more
than one relying party or application, the JWT MUST contain an "aud"
(audience) claim that can be used to determine whether the JWT is
being used by an intended party or was substituted by an attacker at
an unintended party. Furthermore, the relying party or application
MUST validate the audience value and if the audience value is not
present or not associated with the recipient, it MUST reject the JWT.

Just a draft at this point but it may be a useful reference on this and other JWT best practices.

I agree with Brian's comment about versioning. We don't want to break what's currently working for OSG.

@djw8605

This comment has been minimized.

Show comment
Hide comment
@djw8605

djw8605 Sep 19, 2018

Contributor

What is the conclusion on this? I added multiple audience support. Am I clear to merge?

Note: multiple audience support requires a relatively new version of the underlying JWT library, which is newer than the version distributed in EPEL. So I will need to build a new version in the OSG.

Contributor

djw8605 commented Sep 19, 2018

What is the conclusion on this? I added multiple audience support. Am I clear to merge?

Note: multiple audience support requires a relatively new version of the underlying JWT library, which is newer than the version distributed in EPEL. So I will need to build a new version in the OSG.

g_audience = json.loads(cp.get("Global", "audience_json"))
elif 'audience' in cp.options(section):
g_audience = cp.get("Global", "audience")
if ',' in g_audience:

This comment has been minimized.

@bbockelm

bbockelm Sep 21, 2018

Contributor

Can you always split the g_global_audience? This way, g_audience is always a list and never a string.

@bbockelm

bbockelm Sep 21, 2018

Contributor

Can you always split the g_global_audience? This way, g_audience is always a list and never a string.

This comment has been minimized.

@djw8605

djw8605 Sep 21, 2018

Contributor

Sure, I can always make g_audience a list, though the underlying library pyjwt doesn't really care.

@djw8605

djw8605 Sep 21, 2018

Contributor

Sure, I can always make g_audience a list, though the underlying library pyjwt doesn't really care.

@bbockelm

One small code change requested.

Are there any corresponding test cases?

djw8605 added some commits Sep 21, 2018

@djw8605

This comment has been minimized.

Show comment
Hide comment
@djw8605

djw8605 Oct 8, 2018

Contributor

Just to let you know, I'm working on this. Things that need to be done to get this test succesfully working:

  1. Updated python-jwt built in OSG (https://opensciencegrid.atlassian.net/browse/SOFTWARE-3438)
  2. Update to scitokens to support multiple-aud (scitokens/scitokens#82)
  3. Build scitokens in OSG from new release that includes multiple-aud
Contributor

djw8605 commented Oct 8, 2018

Just to let you know, I'm working on this. Things that need to be done to get this test succesfully working:

  1. Updated python-jwt built in OSG (https://opensciencegrid.atlassian.net/browse/SOFTWARE-3438)
  2. Update to scitokens to support multiple-aud (scitokens/scitokens#82)
  3. Build scitokens in OSG from new release that includes multiple-aud
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment