[BUG] "keyInfo is not in PEM format or in base64 format" when upgrading to v5

[forty]

Because of this commit 8920013 some of our certificates which contained spaces caused the error TypeError: keyInfo is not in PEM format or in base64 format, while it was working fine in previous version.

As this breaking change was not explicitly stated in the changelog, I figured out that I'd log this bug in case someone else searches for it. The solution is to clean up the base64 of idpCert before passing it to node-saml (so no spaces, correct "=" padding, etc) - I don't know if node-saml should defensively perform the clean up too, up to you (let me know if you'd like a PR for this, happy to do it).

[forty]

note that I choose the bug category, though it isn't really a bug, because I did not know what else to choose - since the breaking change happened in a major release, this is mostly for documentation purpose.

[cjbarth]

I'd be happy to see what a PR like this would look like. Were the spaces something that were generated by some tool you used or were they are artifact of some other processing you did with them?

[forty]

So I don't really know how those spaces appeared tbh, the certificates are provisioned by our customers and it seems some of them have somehow the end of line of the base64 turned to spaces, and we did not validate enough so we accepted those certificates and stored them as is (which worked fine until the update, so we never realized).

For the "fix" I don't imagine anything too fancy one of:

keyData = keyData.replace(/ /g, ''); // drop only spaces
keyData = keyData.replace(/\s/g, ''); // drop all white-spaces
keyData = keyData.replace(/[^A-Za-z0-9\+\/=]/g,''); // drop all non base64 character

One potential issue (we did not encounter, but I think could happen) would also be missing padding, which probably used to work, but would be blocked by the new stricter regexp. I don't know if it would be a good idea to handle that too.

[cjbarth]

Wouldn't that affect the header line as well?

[forty]

No because the failing assertion is after checking for PEM headers

node-saml/src/crypto.ts

Line 63 in 72a39dc

assertRequired(isBase64 || undefined, "keyInfo is not in PEM format or in base64 format");

so that's were I would do the replace

Our certificate are in base 64 without the PEM headers. When there are PEM header, it seems that the regexp doesn't validate the base 64 at all (.*) so it would also have avoided us the issue

[msavolainen-gofore]

We had the same problem. Our key files had Bag attributes and Key attributes blocks before begin private key line. Removing those lines fixed the issue.

[srd90]

Because of this commit 8920013 some of our certificates which contained spaces caused the error...

8920013 states multiple times that:

...Node-SAML is enforcing RFC7468 stricttextualmsg format for PEM files...

node-saml/README.md

Lines 185 to 186 in 8920013

	To sign authentication requests, private key needs to be provide in the PEM format via the `privateKey` configuration property.
	Node-SAML is enforcing [RFC7468](https://www.rfc-editor.org/rfc/rfc7468) `stricttextualmsg` format for PEM files.

node-saml/README.md

Lines 217 to 219 in 8920013

	It is important to validate the signatures of the incoming SAML Responses.
	For this, provide the Identity Provider's public X.509 signing certificate(s) or public key(s) in [RFC7468](https://www.rfc-editor.org/rfc/rfc7468) `stricttextualmsg` PEM format
	via the `cert` configuration property.

I presume it means ABNF https://www.rfc-editor.org/rfc/rfc7468#section-3 section's Figure 3:

  stricttextualmsg = preeb eol
                     strictbase64text
                     posteb eol

  strictbase64finl = *15(4base64char) (4base64char / 3base64char
                       base64pad / 2base64char 2base64pad) eol

  base64fullline   = 64base64char eol

  strictbase64text = *base64fullline strictbase64finl

                         Figure 3: ABNF (Strict)

@forty you wrote:

...The solution is to clean up the base64 of idpCert before passing it to node-saml (so no spaces, correct "=" padding, etc) - I don't know if node-saml should defensively perform the clean up too, up to you (let me know if you'd like a PR for this, happy to do it).

IMHO question is then whether node-saml should use lax (figure 2) or standard (figure 1) instead of strict and/or whether current implementation follows stricttextualmsg definition correctly along with all MUSTs of RFC7468

RCF7468's General Considerations https://www.rfc-editor.org/rfc/rfc7468#section-2 section contains also discussion of whitespaces.

FWIW, found also this answer which might bring some clarity/insight to whitespace stuff https://stackoverflow.com/a/40404153

[cjbarth]

Thank you @srd90 for this research. It does seem that, while we could allow for more lenient whitespace handing, it cautions that this "may run the risk of accepting text that was not intended to be accepted in the first place". However, "the choice of parsing strategy depends on the context of use"; we can pick and still be compliant. It seems we have picked, and we are compliant, and any cert used which has extraneous whitespace is actually not from a compliant generator.

The quote in that Stack Overflow answer where it says "Furthermore, parsers SHOULD ignore whitespace and other non-base64 characters" is referring to data before the encapsulation boundaries.

Data before the encapsulation boundaries are
permitted, and parsers MUST NOT malfunction when processing such
data. Furthermore, parsers SHOULD ignore whitespace and other non-
base64 characters and MUST handle different newline conventions.

Unless I'm missing something, the approach currently implemented is the most correct one and there isn't a compelling reason to change.

Here are some relevant snippets:

New implementations SHOULD emit the strict format (Figure 3)
specified above. The choice of parsing strategy depends on the
context of use.

Most extant parsers ignore blanks at the ends of lines; blanks at the
beginnings of lines or in the middle of the base64-encoded data are
far less compatible. These observations are codified in Figure 1.
The most lax parser implementations are not line-oriented at all and
will accept any mixture of whitespace outside of the encapsulation
boundaries (see Figure 2). Such lax parsing may run the risk of
accepting text that was not intended to be accepted in the first
place (e.g., because the text was a snippet or sample).

Generators MUST wrap the base64-encoded lines so that each line
consists of exactly 64 characters except for the final line, which
will encode the remainder of the data (within the 64-character line
boundary), and they MUST NOT emit extraneous whitespace. Parsers MAY
handle other line sizes. These requirements are consistent with PEM
[RFC1421].

[KiwiKilian]

While I appreciate the strict format, it would be really helpful to have an error message which actually tells, which key/cert is falsy.

For us it was the idpCert which is externally supplied.

[cjbarth]

That seems very reasonable @KiwiKilian . Can you put up a PR for that?