Csr validator #68
Csr validator #68
Conversation
validator/executable/validator.go
Outdated
| err = cmd.Run() | ||
| if err != nil { | ||
| return false, err | ||
| } else { |
There was a problem hiding this comment.
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.
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.
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.
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.
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, | |||
|
|
||
| err = cmd.Run() | ||
| if err != nil { | ||
| // mask the executable error |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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 | |||
| @@ -0,0 +1,57 @@ | |||
| package executablecsrverifier | |||
| OtherExecute | ||
| ) | ||
|
|
||
| func New(path string) (*ExecutableCSRVerifier, error) { |
| ) | ||
|
|
||
| const ( | ||
| UserExecute os.FileMode = 1 << (6 - 3*iota) |
There was a problem hiding this comment.
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.