CA-404640 XSI-1781 accept in PEM key/cert in any order #6227
Conversation
Can we add a pem file that follows the format and add it to the unit-tests as an expected failure? |
|
@psafont a file containing cert; cert; key does not fail to parse. The reason is that the "until" combinator also skips over certs. So this combinator parsing approach is a bit fragile. |
|
A less fragile approach could be to collect all keys, and all certificates. Then inspect their relationship to block certain inputs:
I'm tempted to start verifying the signing chains, but it's a rabbit hole and not needed for this. |
|
The problem remains that we want to skip over junk but not over blocks of certs and keys; I need to think about how to do that. |
For a long-term option, I'm looking at adding some functions to upstream x509 |
lindig
force-pushed
the
private/christianlin/CA-404640
branch
from
1185300 to
5c61c07
Compare
|
I have changed the approach: now collecting all blocks then picking the ones to use. |
| let strip = function Cert c -> c | Key k -> k in | ||
| blocks >>= fun bs -> | ||
| match List.partition (function Key _ -> true | Cert _ -> false) bs with | ||
| | [Key k], Cert c :: xs -> |
There was a problem hiding this comment.
Is it possible that a pem file has one private key, but no certificate? Do we want to distinguish that failure case?
There was a problem hiding this comment.
This could happen, but it's printed as a "more than one key case", it probably should be extracted
| let strip = function Cert c -> c | Key k -> k in | ||
| blocks >>= fun bs -> | ||
| match List.partition (function Key _ -> true | Cert _ -> false) bs with | ||
| | [Key k], Cert c :: xs -> |
There was a problem hiding this comment.
The code assumes that the first certificate is the one signed by the key. This might not be true.
I think for now this is fine, but it might not always be true
| let _parse_with t path = | ||
| let consume = Consume.Prefix in | ||
| read_file path |> parse_string ~consume t | ||
|
|
There was a problem hiding this comment.
I'm using it for interactive testing in the top level
| many end_of_line *> return {private_key; host_cert; other_certs} <?> "pem" | ||
| let strip = function Cert c -> c | Key k -> k in | ||
| blocks >>= fun bs -> | ||
| match List.partition (function Key _ -> true | Cert _ -> false) bs with |
There was a problem hiding this comment.
this works, although OCaml now has List.partition_map, which together with using polymorphic variants could be used to create 2 distinct list: one that can only contain keys, and one that can only contain certificates, and the typechecker ensures that we don't mix up the two.
ocaml/gencert/pem.ml
Outdated
| | [Key k], Cert c :: xs -> | ||
| return {private_key= k; host_cert= c; other_certs= List.map strip xs} | ||
| | [_], [] -> | ||
| fail "PEM is lacking a certificate" |
There was a problem hiding this comment.
in the failure case it might be useful to print a count of how many blocks we did parse. E.g. if there are no certificates we should print how many private keys it has (maybe it has neither, i.e. we completely failed to parse anything useful).
Perhaps a debug line that logs the output of 'List.partition' (i.e. List.length for each of the 2 lists)?
There was a problem hiding this comment.
I will the byte offset we are at when we fail
There was a problem hiding this comment.
Thinking about this: at this point we parsed all blocks and we will be at the end of the file.
| cert >>= (fun c -> return (Cert c)) <|> (key >>= fun k -> return (Key k)) | ||
|
|
||
| (* this skips over junk until we succeed finding the next block *) | ||
| let block = fix (fun m -> any_block <|> line *> m) <?> "until_block" |
There was a problem hiding this comment.
Maybe not a concern here, since the files are usually small, but should we use the 'commit' function from Angstrom to prevent backtracking too much?
Otherwise with N lines it will attempt 2^N ways to parse it.
Angstrom is not like a usual, efficient, parser that construct a table to scan the input in ~linear time. Worst case it can take 2^N time, where N is the number of alternatives.
Or does the 'fix' function implicitly do this already?
We have so far hard-coded the exectation that in a PEM file a private key is followed by a certficate but this is actually not required by the PEM standard and let to a failure in XSI-1781. This is a simple fix first collects all keys and certificates while skipping over other content and the uses the first key and certificate. Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
lindig
force-pushed
the
private/christianlin/CA-404640
branch
from
dadd40b to
aac5802
Compare
| @@ -1,4 +1,74 @@ | |||
| -----BEGIN RSA PRIVATE KEY----- | |||
There was a problem hiding this comment.
This is removing a failing unit-test of mismatched header and footer, could it be added back?
Add back a unit test. Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
We have so far hard-coded the expectation that in a PEM file a private key is followed by a certificate but this is actually not required by the PEM standard and let to a failure in XSI-1781.
This is a simple fix that permits both. However, it would still fail if we have a certificate with multiple certificates followed by a key. Another strategy could be to parse all blocks in a PEM file and to pick out a key and certificate later rather than expecting a particular order up front.
Added new test data for unit tests; fail-01.pem now becomes a passing test case.