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 JWT support #85

Merged
merged 36 commits into from
Dec 12, 2023
Merged

Add JWT support #85

merged 36 commits into from
Dec 12, 2023

Conversation

JU4N98
Copy link
Contributor

@JU4N98 JU4N98 commented Jul 19, 2023

Changes proposal:

These changes allow JWT svids and JWK bundles getting by specifying an audience, JWT file name and JWK file name.

fixes #43

pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
pkg/sidecar/config.go Outdated Show resolved Hide resolved
pkg/sidecar/config.go Outdated Show resolved Hide resolved
pkg/sidecar/config.go Outdated Show resolved Hide resolved
@keeganwitt
Copy link
Contributor

config.go is still requiring svid_file_name and related values be set, which aren't necessary when just using the JWT functionality.

pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
pkg/sidecar/config.go Outdated Show resolved Hide resolved
@faisal-memon
Copy link
Collaborator

config.go is still requiring svid_file_name and related values be set, which aren't necessary when just using the JWT functionality.

Good catch @keeganwitt

pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
@JU4N98
Copy link
Contributor Author

JU4N98 commented Nov 6, 2023

config.go is still requiring svid_file_name and related values be set, which aren't necessary when just using the JWT functionality.

Good catch @keeganwitt

Should we validate that x509, JWT or JWT bundles are going to be fetched? Or simply delete this validation?

README.md Outdated Show resolved Hide resolved
@keeganwitt
Copy link
Contributor

config.go is still requiring svid_file_name and related values be set, which aren't necessary when just using the JWT functionality.

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")
}

@JU4N98
Copy link
Contributor Author

JU4N98 commented Nov 7, 2023

config.go is still requiring svid_file_name and related values be set, which aren't necessary when just using the JWT functionality.

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.

README.md Outdated Show resolved Hide resolved
pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
pkg/sidecar/util_posix.go Outdated Show resolved Hide resolved
pkg/sidecar/util_windows.go Outdated Show resolved Hide resolved
JU4N98 and others added 23 commits December 5, 2023 10:55
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]>
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]>
@JU4N98
Copy link
Contributor Author

JU4N98 commented Dec 5, 2023

@JU4N98 There are 5 open comments left. Please let us know when you have updated.

I think is ready for a last review @faisal-memon , @maxlambrecht and @MarcosDY

Copy link
Collaborator

@faisal-memon faisal-memon left a 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?

Copy link
Member

@maxlambrecht maxlambrecht left a comment

Choose a reason for hiding this comment

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

Thanks, @JU4N98

@faisal-memon faisal-memon merged commit 3ba5715 into spiffe:main Dec 12, 2023
12 checks passed
@keeganwitt keeganwitt mentioned this pull request Dec 13, 2023
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.

add support for jwt-svid
6 participants