Fix CRL conversion issue when already in PEM format

[teki69]

If downloaded CRL file is already in PEM format, openssl command will fail as it is always trying to convert from a DER-encoded CRL. This commit is for adding a test of the CRL format prior to running the openssl crl conversion.

Note: as the openssl verify command then assumes that a .pem tmpfile has been generated by the conversion, there would be an issue when the conversion was not needed (i.e. CRL already PEM-encoded) as that .pem would be missing; therefore I also added a copy of the .crl file to a .crl.pem file before the optional conversion.

[dcooper16]

Hello @teki69,

Thanks for the PR. It may be a good idea to detect if the returned CRL is PEM encoded. However, I personally do not like the idea of having testssl.sh silently accept PEM-encoded CRLs. According to RFC 5280:

If the DistributionPointName contains a general name of type URI, the following semantics MUST be assumed: the URI is a pointer to the current CRL for the associated reasons and will be issued by the associated cRLIssuer. When the HTTP or FTP URI scheme is used, the URI MUST point to a single DER encoded CRL as specified in [RFC2585].

So, if the server is returning a PEM-encoded CRL, it is incorrectly configured. Thus, in my opinion, testssl.sh should issue a warning in this case. Certainly making the warning more clear than "conversion of \"$tmpfile\" failed" in the case that we know the issue is that the CRL is PEM encoded would be a useful change. I think it would be okay to continue to use that CRL with the $OPENSSL verify command, as long as there is a warning.

As for the PR as it is currently works, I have just a couple of comments.

First, the diff makes it appear that the entire file has changed. This is an issue that others have experienced recently (e.g., #2360 (comment)), so perhaps someone else can provide provide a suggestion on how the fix the problem.

Second, your PR is the following:

cp "$tmpfile" "${tmpfile%%.crl}.pem"
grep -qe 'BEGIN X509 CRL' "${tmpfile%%.crl}.pem" || $OPENSSL crl -inform DER -in "$tmpfile" -outform PEM -out "${tmpfile%%.crl}.pem" &>$ERRFILE

This performs an unnecessary "cp" in the common case in which the server is correctly configured, and file operations such as "cp" are relatively expensive.

It would be more efficient to do something like:

if grep -qe 'BEGIN X509 CRL' "$tmpfile"; then
    mv "$tmpfile" "${tmpfile%%.crl}.pem"
else
     $OPENSSL crl -inform DER -in "$tmpfile" -outform PEM -out "${tmpfile%%.crl}.pem" &>$ERRFILE`
      if [[ $? -ne 0 ]]; then
           pr_warning "conversion of \"$tmpfile\" failed"
           fileout "$jsonID" "WARN" "conversion of CRL to PEM format failed"
           return 1
      fi
fi

Perhaps an even better option would be to try converting to PEM, and then only if that fails check whether the file is already PEM encoded. That way that no extra cost is paid by correctly configured servers.

$OPENSSL crl -inform DER -in "$tmpfile" -outform PEM -out "${tmpfile%%.crl}.pem" &>$ERRFILE
if [[ $? -ne 0 ]]; then
     if grep -qe 'BEGIN X509 CRL' "$tmpfile"; then
          mv "$tmpfile" "${tmpfile%%.crl}.pem"
     else
          pr_warning "conversion of \"$tmpfile\" failed"
          fileout "$jsonID" "WARN" "conversion of CRL to PEM format failed"
          return 1
     fi
fi

[drwetter]

Hi @teki69 ,

thanks. I can't read your PR through the web interface.

Did you pull the patch, @dcooper16 or how did you do that?

[dcooper16]

Did you pull the patch, @dcooper16 or how did you do that?

I downloaded just the testssl.sh file, ran dos2unix on it, and then performed diff locally.

[teki69]

Thanks for your answers, and I fully agree with you @dcooper16 and your code proposal.
As regard to the diff issue, this is weird as I had only changed 2 lines from the original file that I modified directly with github.com file editor from your repo. Then github made me fork it so that I can commit the changes and I created a PR. So everything was done online from github.

[drwetter]

Using the online editor I had this kind of an "experience" recently too. There I changed only a single comment.

Don't know why github is doing this yet but we need to find out.

[teki69]

I've added a new commit in the PR, that includes @dcooper16 's code proposal. This time I've edited the file outside github and converted it to unix's newline format, before uploading it in my repo. Now it seems that the PR diff shows correctly in github web interface:

[drwetter]

Thanks to the both of you! Will merge it when the CI run finished.

[drwetter]

Just a note here: I was hoping defining a file which extension has which line endings via .gitattributes (#2364) PR will help but it seems it did not: https://github.com/drwetter/testssl.sh/pull/2365/files

[teki69]

Just a note here: I was hoping defining a file which extension has which line endings via .gitattributes (#2364) PR will help but it seems it did not: https://github.com/drwetter/testssl.sh/pull/2365/files

Since your .gitattributes file doesn't have the default * text=auto it may be required that you specify text explicitly for each extension, e.g. *.sh text eol=lf

If that could be useful, there's a repo providing plenty of .gitattributes templates depending on the languages; it contains a general common template that includes .sh extension. I've found the repo link in this article (in french, sorry) regarding git files normalization.

[drwetter]

Since your .gitattributes file doesn't have the default * text=auto it may be required that you specify text explicitly for each extension, e.g. *.sh text eol=lf

"Maybe" is correct. At least I wasn't sure we need it as I saw reference with and without text. The french text also says that what I kind f knew before "S’il n’est pas spécifié pour un type de fichier, Git utilise alors le comportement par défaut, " --> if it was not specified what for a type of file (it is), git uses the normal one per default.