-
Notifications
You must be signed in to change notification settings - Fork 167
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
Added --client-signing-algorithms flag #1974
base: main
Are you sure you want to change the base?
Added --client-signing-algorithms flag #1974
Conversation
ee1f9ae
to
be96ecd
Compare
be96ecd
to
b450afb
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1974 +/- ##
===========================================
- Coverage 66.46% 49.88% -16.58%
===========================================
Files 92 192 +100
Lines 9258 24857 +15599
===========================================
+ Hits 6153 12401 +6248
- Misses 2359 11341 +8982
- Partials 746 1115 +369
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bffe52e
to
55f01c3
Compare
Apparently the e2e test does not fill the Signature part of the JARModel: if v.JARModel.Signature == nil || v.JARModel.Signature.PublicKey == nil || v.JARModel.Signature.PublicKey.Content == nil {
return nil, errors.New("jar v0.0.1 entry not initialized")
} Shall this be handled or is the signature always expected to be present? |
Signature should be present, it's required to validate a new jar entry - rekor/pkg/types/jar/v0.0.1/entry.go Line 248 in d596e9d
|
2ac074e
to
34f31c3
Compare
Signed-off-by: Riccardo Schirone <[email protected]>
808f8d9
to
c31e028
Compare
pkg/api/entries.go
Outdated
// Default to SHA256 if no artifact hash is specified | ||
artifactHashValue := crypto.SHA256 | ||
// Get artifact hash from entry | ||
artifactHash, err := entry.ArtifactHash() |
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.
what if err != nil
?
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.
refactored this a bit, let me know if it's better!
|
||
areEntryAlgorithmsAllowed, err := checkEntryAlgorithms(entry) | ||
if err != nil { | ||
return nil, handleRekorAPIError(params, http.StatusBadRequest, err, fmt.Sprintf(validationError, err)) | ||
} | ||
if !areEntryAlgorithmsAllowed { | ||
return nil, handleRekorAPIError(params, http.StatusBadRequest, errors.New("entry algorithms are not allowed"), fmt.Sprintf(validationError, "entry algorithms are not allowed")) | ||
} | ||
|
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.
this feels like it should be inside types.CreateVersionedEntry
instead of in the API layer; wdyt?
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.
Maybe, but it uses api.algorithmRegistry
and nothing in pkg/types use api
. Would that be fine anyway?
Signed-off-by: Riccardo Schirone <[email protected]>
// Only check algorithms for hashedrekord entries | ||
switch entry.(type) { | ||
case *hashedrekord.V001Entry: | ||
break | ||
default: | ||
return true, nil | ||
} | ||
|
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.
@bobcallaway @haydentherapper shall we start with hashedrekord entries only? If we want to add support for other types I guess we need to change the schemas similarly to what we did for hashedrekord.
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.
My 0.02c is that it's fine to start here: my understanding is that the long term plan is to remove a lot of the non-hashed entries with Rekor v2 anyways, plus this flag is primarily for test deployment purposes anyways 🙂.
Signed-off-by: Riccardo Schirone <[email protected]>
Signed-off-by: Riccardo Schirone <[email protected]>
74d6afa
to
7a031f8
Compare
code looks fine. I would like to see some e2e tests that show it works before merging. |
4cbdc5d
to
2899d57
Compare
@bobcallaway I added an e2e test, though I don't see it running in the CI. Could it be because it's not in |
It requires https://github.com/sigstore/rekor/actions/runs/13202747393/job/36858854058?pr=1974 |
Signed-off-by: Riccardo Schirone <[email protected]>
2899d57
to
e2e4a9b
Compare
Summary
Add
--client-signing-algorithms
flag to rekor-server to restrict the set of client keys accepted by a Rekor instance. See #1724 .This work depends on sigstore/sigstore#1601
Release Note
Documentation