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

Add support for secondary auth in IdentityCredentials #382

Merged
merged 10 commits into from
May 11, 2023

Conversation

potto007
Copy link
Contributor

@potto007 potto007 commented Apr 25, 2023

WIP: exploring how to add Secondary Credentials support in the least obtrusive manner

@andrewpmartinez
Copy link
Member

andrewpmartinez commented Apr 25, 2023

Can you elaborate on your use case? Are you trying to figure out how to authenticate w/ certificates and also provide a JWT? (I mention this because this is the only case that would need a primary auth w/ a secondary).

@potto007
Copy link
Contributor Author

Can you elaborate on your use case? Are you trying to figure out how to authenticate w/ certificates and also provide a JWT? (I mention this because this is the only case that would need a primary auth w/ a secondary).

Hey @andrewpmartinez - yes, I am finishing up making authentication with primary and secondary work when secondary is an external jwt. I tested it against the Controller several weeks ago using Postman, and verified that the auth works when using a cert identity along with a JWT bearer on every request.

I realized I went a bit too far on this new type, and I'm pairing it down to just augment IdentityCredentials to include a JWT field.

@potto007
Copy link
Contributor Author

Cleaning up, removing the custom type and instead overloading the IdentityCredentials type with optional JWT header support.

@potto007 potto007 changed the title Add new credentials type: SecondaryCredentials Add support for secondary auth in IdentityCredentials Apr 28, 2023
@potto007
Copy link
Contributor Author

This update to sdk-golang now simply adds to the IdentityCredentials type. An example of its use can be seen in the zssh PR I made: openziti-test-kitchen/zssh#31

The existing functionality in Edge is such that, at the end of the cert auth method is processing, the code checks for an auth policy, and if the auth policy has external JWT as a secondary auth, it goes on to check for a token in the header, validates it, and succeeds if everything is in order.

@potto007 potto007 marked this pull request as ready for review April 28, 2023 17:32
@potto007 potto007 requested a review from a team as a code owner April 28, 2023 17:32
@potto007 potto007 force-pushed the add-secondary-auth branch from ffe6698 to f1fd59d Compare April 28, 2023 20:44
ziti/contexts.go Outdated Show resolved Hide resolved
Copy link
Member

@andrewpmartinez andrewpmartinez left a comment

Choose a reason for hiding this comment

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

I think this would be better as an improvement to BaseCredentials instead and de-JWT'ed. Rather add a Headers map[string][]string{}. Then augment the BaseCredentials.AuthenticateRequest to apply those headers to the runtime.ClientRequest. Then alter all the other credentials first to call self.BaseCredentials.AuthenticateRequest(cr, r) so that their own logic applies afterward. The existing JWT authenticator can instead set the underlying headers field.

The result is that credentials instantiations can do something like:

jwtStr := getJwt()
cred := NewIdentityCrednetialFromConfig(config)
cred.Headers["authorization"][]string{"Bearter " + jwtStr}

...or any other header that comes along.

Reasoning:

  1. Placing the functionality here limits the applicability to other cred mechanisms
  2. Other authentication mechanisms are coming that are not JWTs but have header implications

@potto007
Copy link
Contributor Author

potto007 commented May 2, 2023

That would be so much nicer! I'll give that a shot.

@potto007 potto007 force-pushed the add-secondary-auth branch from 988cc20 to 9b68a6e Compare May 4, 2023 14:23
@potto007 potto007 requested a review from andrewpmartinez May 4, 2023 14:26
@potto007
Copy link
Contributor Author

potto007 commented May 4, 2023

@andrewpmartinez I've refactored credentials.go based on your review.

@potto007 potto007 force-pushed the add-secondary-auth branch from 9b68a6e to 7b03987 Compare May 4, 2023 15:50
@andrewpmartinez
Copy link
Member

Gonna look at this today.

ziti/contexts.go Outdated Show resolved Hide resolved
Copy link
Member

@andrewpmartinez andrewpmartinez left a comment

Choose a reason for hiding this comment

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

See comments.

@potto007
Copy link
Contributor Author

@andrewpmartinez any thoughts?

@potto007 potto007 force-pushed the add-secondary-auth branch from cae9141 to da8597e Compare May 10, 2023 15:29
@andrewpmartinez andrewpmartinez merged commit 48e11fb into openziti:main May 11, 2023
@potto007 potto007 deleted the add-secondary-auth branch May 11, 2023 21:19
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.

2 participants