Server certificate validation #4015
Conversation
This will help adapt some of the existing functions to server certificates as well as the current trusted ones. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
The function returns the private key for further validation against certificates. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
This includes both the leaf server certificate as well as the chain certificate. This validation is meant to be used offline and does _not_ do a proper, full validation of the certificate chain. This is meant to be used as a check before installing a certificate in dom0. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
psafont
force-pushed
the
server-cert-validation
branch
from
529b432
to
4771575
Compare
ocaml/xapi/certificates.ml
Outdated
| X509.Private_key.decode_pem raw_pem | ||
| |> R.reword_error | ||
| (fun (`Msg err_msg) -> | ||
| if Astring.String.is_infix ~affix:"multi-prime RSA" err_msg then |
There was a problem hiding this comment.
Are there some more specific error variants available? I'd avoid matching on error strings if possible as these might change between versions (although you have a unit test so we'd catch it)
There was a problem hiding this comment.
they are all defined as parse_error "message", unfortunately
| let ensure_keys_match private_key certificate = | ||
| let public_key = X509.Certificate.public_key certificate in | ||
| match public_key, private_key with | ||
| | `RSA pub, `RSA priv when pub = Nocrypto.Rsa.pub_of_priv priv -> |
There was a problem hiding this comment.
Is polymorphic equality the only way to compare these, or would the library provide a comparison function?
There was a problem hiding this comment.
The certificate module provides
val supports_keytype : t -> key_type -> bool
supports_keytype certificate key_type is result, whether public key of the certificate matches the given key_type.
But I don't see a function to extract keytype from a private or public key. I don't think I can avoid polymorphic equality.
(on the x509 tests it's done the same way)
| in | ||
|
|
||
| let ensure_validity ~time certificate = | ||
| let to_string = Ptime.to_rfc3339 in |
There was a problem hiding this comment.
Should we also include some grace period here?
i.e. if someone tries to install a certificate that expires the next day we should probably warn them in that case too?
i.e. have a minimum validity period of lets say 30 days?
There was a problem hiding this comment.
There are alerts planned to warn users about expiring server certificates, I'm not sure if it's worth doing the check here as well. I'll take note and see if it's easy to reuse those checks here as well.
|
I want the DCO check to fail with the travis commit on the feature branch as it needs to be removed when merging to master. |
…e merging to master)
psafont
force-pushed
the
server-cert-validation
branch
from
5129dc7
to
0bf85cd
Compare
| let stat = Unix.stat (library_path kind) in | ||
| let mask = 0o666 in | ||
| let perm = stat.Unix.st_perm land mask in | ||
| debug "%d %d" perm stat.Unix.st_perm; |
There was a problem hiding this comment.
Has this log line enough context to be useful? These are just two numbers.
There was a problem hiding this comment.
I don't think so, I'm partial to removing the logline.
I changed this function because it's most probably going to be reused for the new certificates, which need different permissions.
psafont
force-pushed
the
server-cert-validation
branch
from
4f6565b
to
29d48ae
Compare
Previously hostname could be empty if there were no hostnames available in the certificate. Now the hostname from xapi's database is picked instead, xapi_vdi was adapted. Signing certificates needs the usage of result as it verifies before actually signing the certificate, test code was adapted. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
psafont
force-pushed
the
server-cert-validation
branch
from
29d48ae
to
28bbb78
Compare
|
Merging this now that the code compatible with X509 0.9.0. I'll keep the comments in mind for doing further cleanups in certificates.ml |
Add private key and certificate sanity checking checking, this is not meant to replace online certificate chain checking.
I also took the opportunity to make the existing interface clearer.