-
Notifications
You must be signed in to change notification settings - Fork 136
Add CertsSelector option #147
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
Conversation
@jessepeterson WDYT? |
@pavolmarko FYI! |
scep/certs_selector.go
Outdated
} | ||
|
||
// NopCertsSelector is a CertsSelector that does not do anything. | ||
type NopCertsSelector struct{} |
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.
Just a nit, but I kinda like the HandlerFunc
pattern over the empty structs. I.e.:
type CertsSelectorFunc func([]*x509.Certificate) []*x509.Certificate
func (f *CertsSelectorFunc) SelectCerts(certs []*x509.Certificate) []*x509.Certificate {
return f(certs)
}
And then NopSelector becomes:
func NopCertsSelector() CertsSelectorFunc {
return func(certs []*x509.Certificate) []*x509.Certificate {
return certs
}
}
Thoughts?
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.
Done
I am still super new to Go, so you may find me adapting best practices of other languages. Thanks for teaching me the Go way of doing things :-)
import "crypto/x509" | ||
|
||
// CertsSelectorFunc is a type of function that filters certificates. | ||
type CertsSelectorFunc func([]*x509.Certificate) []*x509.Certificate |
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.
Oh, sorry! Didn't mean to get rid of the CertsSelector
interface. Just that CertsSelectorFunc
satisfies the CertsSelector
interface. So folks can provide the implementation that they want.
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.
Nice one!!
Done!
As mentioned in #145, some CAs don't have
KeyUsageKeyEncipherment
key usage set in their certificates. This PR introducesCertsSelector
which should make it easier for clients that want to enforce a custom criteria on how to filter certificates.