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

Adds initial implementation of the JWT package #15

Merged
merged 13 commits into from
Jan 22, 2021
Merged

Conversation

austingebauer
Copy link
Member

@austingebauer austingebauer commented Jan 14, 2021

Description

This PR adds an initial implementation of the JWT package, which provides signature verification and claims set validation for JWTs of the JWS form.

The code in this package is similar to that in both the vault-plugin-auth-jwt and consul JWT auth implementations.

Testing

I've integrated and tested the code in this PR in the common-jwt-lib branch of vault-plugin-auth-jwt.

Tests are intentionally absent from this PR in order to first agree on the API and behavior of the package. I'll be adding tests and additional documentation in a subsequent PR once there is agreement.

@jimlambrt
Copy link
Collaborator

Just an overall observation about errors in the package: as implemented callers are forced to use string matching to test for different errors. perhaps, we should define a set of testable errors (via errors.Is) and always wrap them with context info before returning. More aligned with how the oidc package does this.

Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

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

Looks good... just a few suggestions and I'm happy to discuss in real-time.

jwt/jwt.go Show resolved Hide resolved
jwt/jwt.go Outdated Show resolved Hide resolved
jwt/jwt.go Show resolved Hide resolved
jwt/jwt.go Show resolved Hide resolved
jwt/jwt.go Outdated Show resolved Hide resolved
jwt/keyset.go Outdated Show resolved Hide resolved
jwt/keyset.go Outdated Show resolved Hide resolved
jwt/keyset.go Show resolved Hide resolved
jwt/keyset.go Outdated Show resolved Hide resolved
jwt/keyset.go Outdated Show resolved Hide resolved
jwt/jwt.go Outdated Show resolved Hide resolved
jwt/jwt.go Show resolved Hide resolved
jwt/keyset.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

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

Looks great. One very small suggestion/nit.

jwt/jwt.go Show resolved Hide resolved
jwt/keyset.go Outdated Show resolved Hide resolved
@jimlambrt jimlambrt self-requested a review January 21, 2021 18:12
Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

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

Looks great. ty!

jwt/jwt.go Outdated Show resolved Hide resolved
jwt/keyset.go Outdated Show resolved Hide resolved
jwt/keyset.go Outdated Show resolved Hide resolved
@austingebauer austingebauer merged commit 1e16050 into main Jan 22, 2021
@austingebauer austingebauer deleted the jwt-initial branch January 22, 2021 19:08
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.

4 participants