-
Notifications
You must be signed in to change notification settings - Fork 130
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
Csr validator #68
Csr validator #68
Conversation
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.
A few nits but overall this is exactly what we need.
validator/executable/validator.go
Outdated
err = cmd.Run() | ||
if err != nil { | ||
return false, err | ||
} else { |
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.
It's customary to drop the else in Go if you're going to return in the previous branch.
server/service.go
Outdated
@@ -91,8 +93,21 @@ func (svc *service) PKIOperation(ctx context.Context, data []byte) ([]byte, erro | |||
|
|||
// validate challenge passwords | |||
if msg.MessageType == scep.PKCSReq { | |||
if !svc.challengePasswordMatch(msg.CSRReqMessage.ChallengePassword) { | |||
CSRIsValid := true |
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.
Should start at false so we don't accidentally sign the csr.
validator/executable/validator.go
Outdated
"os/exec" | ||
) | ||
|
||
func NewExecutableValidator(path string) (*executableValidator, error) { |
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.
executablevalidator .NewExecutableValidator
stutters. How about executablevalidator.New
validator/executable/validator.go
Outdated
return &executableValidator{executable: path}, nil | ||
} | ||
|
||
type executableValidator 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.
This type should be exported for documentation and because it's hard to deal with unexported types (say you wanted to embed it as a field in your struct.
validator/validator.go
Outdated
|
||
// Verify the CSR Raw request | ||
type Validator interface { | ||
Verify(data []byte) (bool, error) |
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.
The method name should match the interface name. Let's go with
+ Verify(data []byte) (bool, error)
scep/scep.go
Outdated
@@ -345,6 +347,7 @@ func (msg *PKIMessage) DecryptPKIEnvelope(cert *x509.Certificate, key *rsa.Priva | |||
return errors.Wrap(err, "scep: parse challenge password in pkiEnvelope") | |||
} | |||
msg.CSRReqMessage = &CSRReqMessage{ | |||
Raw: msg.pkiEnvelope, |
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 this encrypted or decrypted?
|
||
err = cmd.Run() | ||
if err != nil { | ||
// mask the executable error |
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.
To avoid 500 errors and return a 400.
|
||
err = cmd.Run() | ||
if err != nil { | ||
// mask the executable error |
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.
It will be hard to debug the reason this is failing if you hide the error.
Add a logger to the struct and log the error here.
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.
The thing is "exit 1" is the standard behavior, if we want to stay compatible with the puppet interface. We could reroute the stdout / stderr of the script maybe.
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.
exit 1 is fine, but cmd.Run
can fail for other reason. Imagine that I didn't set the executable flag on the binary or something. Masking errors should at least be logged because trying to understand why the run is failing for the user is tough, especially over github/slack.
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.
OK, will try to log it correctly.
BTW, The checks for the executable flags are done in csrexecutableverifier.New
. I could still fail though.
server/service.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
if result { |
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.
how about
CSRIsValid = result
if !result {
svc.debugLogger.Log("err", "CSR is not valid")
}
Reduces the need for the extra branching in the code.
server/service.go
Outdated
svc.debugLogger.Log("err", "CSR is not valid") | ||
} | ||
} else { | ||
if svc.challengePasswordMatch(msg.CSRReqMessage.ChallengePassword) { |
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.
same here.
CSRIsValid = svc.chall...
if !CSRIsValid {
svc.debugLogger.Log ...
}
csrverifier/csrverifier.go
Outdated
@@ -0,0 +1,7 @@ | |||
// Package csrverifier defines an interface for CSR verification |
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.
Add punctuation to package doc.
@@ -0,0 +1,57 @@ | |||
package executablecsrverifier |
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.
needs doc
OtherExecute | ||
) | ||
|
||
func New(path string) (*ExecutableCSRVerifier, error) { |
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.
Exported functions need doc
) | ||
|
||
const ( | ||
UserExecute os.FileMode = 1 << (6 - 3*iota) |
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.
Do you think these constants could be private?
Less branching Some constants made private Add doc
This is a great enhancement. Thanks for sticking through code review with me! |
Probably not the code quality you are excepting. Bear with me, I am a go newbie. This is more a proof of concept, so that we can continue the discussion about the issue #64.
It works currently on my dev machine, and I was able to call a Zentral API to verify the CSR with a little python script. We will publish that soon.