Option to validate only CA and Issuer rather than entire SCEP cert#700
Option to validate only CA and Issuer rather than entire SCEP cert#700grahamgilbert merged 5 commits intomainfrom
Conversation
server/devicecert.go
Outdated
| } | ||
|
|
||
| func VerifyCertificateMiddleware(store ScepVerifyDepot, logger log.Logger) mdm.Middleware { | ||
| func VerifyCertificateMiddleware(validateSCEPIssuer bool, scepIssuer string, scepDepot *boltdepot.Depot, store ScepVerifyDepot, logger log.Logger) mdm.Middleware { |
There was a problem hiding this comment.
Instead of adding this dependency, modify the ScepVerifyDepot interface to add the missing CA method.
We already pass the depot as the store argument, it's just missing a method.
server/devicecert.go
Outdated
|
|
||
| if expiration.After(time.Now()) { | ||
| err := errors.New("device certificate is expired") | ||
| return false, err |
There was a problem hiding this comment.
just return false, errors.New(the text). same for all the other instances where you create err := just to return it on the next line.
server/devicecert.go
Outdated
| if mw.validateSCEPIssuer { | ||
| verifiedIssuer, err = verifyIssuer(devcert, mw.scepIssuer, mw.scepDepot) | ||
| if err != nil { | ||
| _ = level.Info(mw.logger).Log("err", err, "issuer", devcert.Issuer.String(), "expiration", devcert.NotAfter) |
There was a problem hiding this comment.
remove the _ =, it's not necessary to check for the error on the logs.
server/devicecert.go
Outdated
| verifiedIssuer, err = verifyIssuer(devcert, mw.scepIssuer, mw.scepDepot) | ||
| if err != nil { | ||
| _ = level.Info(mw.logger).Log("err", err, "issuer", devcert.Issuer.String(), "expiration", devcert.NotAfter) | ||
| return nil, errors.Wrap(err, "error verifying CN") |
There was a problem hiding this comment.
Leave the returned error as errors.New("unauthorized client"), and only expand on logs.
The actual error goes to the client, which would leak too much details to an attacker about why the validation is failing.
server/devicecert.go
Outdated
| @@ -41,9 +85,20 @@ func (mw *verifyCertificateMiddleware) Acknowledge(ctx context.Context, req mdm. | |||
| return nil, errors.Wrap(err, "error checking device certificate") | |||
| } | |||
| if !hasCN { | |||
There was a problem hiding this comment.
Use an early return here which allows you to use the scep issuer validation without the extra conditionals.
if !hasCN && !validate // return
server/devicecert.go
Outdated
| @@ -58,9 +113,20 @@ func (mw *verifyCertificateMiddleware) Checkin(ctx context.Context, req mdm.Chec | |||
| return errors.Wrap(err, "error checking device certificate") | |||
| } | |||
| if !hasCN { | |||
There was a problem hiding this comment.
same comments about flow here as in the other verify function.
server/devicecert.go
Outdated
| issuer := devcert.Issuer.String() | ||
| expiration := devcert.NotAfter | ||
|
|
||
| if time.Now().After(expiration) { |
There was a problem hiding this comment.
I would either add an option to not validate the device certificate expiry or don't do it by default. This will, effectively, "lock out" devices from being remediated by re-enrollment if they ever expire and, by default, MicroMDM has a 1yr expiration on the device cert — this could catch many folks off-guard and be in an unfixable state.
groob
left a comment
groob
left a comment
There was a problem hiding this comment.
Agree with the comment from Jesse, otherwise I'm happy with this. Can you also update CHANGELOG for the release notes?
|
Changelog updated, added a flag to opt in to checking if the cert is expired. |
3 participants
Currently we're only verifying whether the cert presented is present in the database. For reasons I don't understand / have time to investigate, clients have valid SCEP certificates which aren't stored in the database. This PR adds an option to validate only the Issuer and the CA that signed the cert.