-
Notifications
You must be signed in to change notification settings - Fork 108
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
Pluggable verifiers #84
Conversation
|
||
// Verifiers serves as a map of all verifiers available on the system and | ||
// can be injected into a verificationService. For testing and configuration | ||
// purposes, it will not be used by default. |
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.
Is the last sentence accurate?
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.
Ah no. It was at one point, then I reverted to a simpler implementation. Will fix.
One thought, the Ed25519 verifier is covered by existing tests. I'll get tests for the RSA verifier eventually but we can take it out for now and leave additional verifiers to the whim of other users. Let me know if you want to do that. I can also just add a comment for now marking the RSA verifier as "untested, use at your own risk" (it does work though, we used it in a demo) |
Yeah, add a comment to the effect of "RSA should only be used for compatibility, this verifier is not currently covered by tests, use at your own risk if you really know what you are doing" |
) | ||
|
||
// The Verifier interface provides flexibility over exactly how a verifier | ||
// is implemented vs having a simple function type. |
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 comment should describe what a Verifier
does instead of comparing implementation options.
Implementation LGTM. Update the comments and squash, and I'll merge. Thanks! |
Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)
…eframework#84) Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.15.0 to 0.16.0. - [Commits](golang/crypto@v0.15.0...v0.16.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.15.0 to 0.16.0. - [Commits](golang/crypto@v0.15.0...v0.16.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
There needs to be an update to the keys.Key type to fully support other algorithms. Happy to make those changes if you can provide an opinion. On my fork I just generically treat all keys and signatures as
[]byte
and allow the creators/verifiers to worry about precisely what the correct sizes are and copy the slices into arrays.Signed-off-by: David Lawrence david.lawrence@docker.com (github: endophage)