spiffe / spiffe-helper

The SPIFFE Helper is a tool that can be used to retrieve and manage SVIDs on behalf of a workload
Apache License 2.0
43 stars 40 forks source link

Add JWT support #85

Closed JU4N98 closed 8 months ago

JU4N98 commented 1 year ago

Changes proposal:

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

fixes #43

keeganwitt commented 9 months ago

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

faisal-memon commented 9 months ago

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

JU4N98 commented 9 months ago

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?

keeganwitt commented 9 months ago

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 commented 9 months ago

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.

keeganwitt commented 9 months ago

TestParseConfig() in config_test.go is missing new options.

maxlambrecht commented 9 months ago

LGTM. @maxlambrecht what do you think?

Looking good, I left a few more nit comments. I'm ready to approve.

JU4N98 commented 9 months ago

LGTM. @maxlambrecht what do you think?

Looking good, I left a few more nit comments. I'm ready to approve.

faisal-memon commented 8 months ago

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

JU4N98 commented 8 months ago

@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