-
Notifications
You must be signed in to change notification settings - Fork 40
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 JWT support #85
Add JWT support #85
Conversation
7cafe1e
to
075ad6e
Compare
075ad6e
to
ed6cb00
Compare
1152082
to
fa6a696
Compare
config.go is still requiring |
Good catch @keeganwitt |
Should we validate that x509, JWT or JWT bundles are going to be fetched? Or simply delete this validation? |
I was thinking if c.SvidFileName == "" && c.JWTFilename == "" {
return errors.New("svid_file_name and/or jwt_file_name is required")
}
if c.SvidFileName != "" && c.SvidKeyFileName == "" {
return errors.New("svid_key_file_name is required when using svid_file_name")
}
if c.SvidFileName != "" && c.SvidBundleFileName == "" {
return errors.New("svid_bundle_file_name is required when using svid_file_name")
}
if c.JWTFilename != "" && c.JWKFilename == "" {
return errors.New("jwk_file_name is required when using jwt_file_name")
} |
Well I think there are three options, fetch X509 SVIDs, fetch JWT SVID or fetch JWT Bundle. So first and fourth conditions should be: if c.SvidFileName == "" && c.JWTFIlename == "" && c.JWKFilename == "" {
// error message
}
if c.JWTFileName != "" && c.JWTAudience == "" {
// error message
} Besides of that, in util_posix.go and util_windows.go, if some of this conditions aren't met no routines will be running. So maybe is better to delete these validations. |
f8172ef
to
2a7f271
Compare
Signed-off-by: JU4N98 <[email protected]>
Co-authored-by: Faisal Memon <[email protected]> Signed-off-by: Juan Pablo Cabaña <[email protected]>
Co-authored-by: Faisal Memon <[email protected]> Signed-off-by: Juan Pablo Cabaña <[email protected]>
Co-authored-by: Faisal Memon <[email protected]> Signed-off-by: Juan Pablo Cabaña <[email protected]>
Co-authored-by: Faisal Memon <[email protected]> Signed-off-by: Juan Pablo Cabaña <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Co-authored-by: Faisal Memon <[email protected]> Signed-off-by: Juan Pablo Cabaña <[email protected]>
Co-authored-by: Faisal Memon <[email protected]> Signed-off-by: Juan Pablo Cabaña <[email protected]>
Co-authored-by: Max Lambrecht <[email protected]> Signed-off-by: Juan Pablo Cabaña <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
20bad39
to
cd61f89
Compare
I think is ready for a last review @faisal-memon , @maxlambrecht and @MarcosDY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the request changes @JU4N98 . This looks good to me. @MarcosDY @maxlambrecht are we good to merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @JU4N98
Changes proposal:
These changes allow JWT svids and JWK bundles getting by specifying an audience, JWT file name and JWK file name.
fixes #43