Patch and enable tests with AWS-LC

[samuel40791765]

Second follow up from #833

This contribution should be the next and final phase for adding support for AWS-LC in the ruby/openssl gem. These changes represent the build portion of a larger patch in the AWS-LC's Ruby integration CI. With this change, all tests have been accounted for and reenabled in the CI.

I understand that this patch is a bit large, so I've broken everything up to into smaller documented commits that'll make things easier to digest. I've also tried following a similar commit messaging pattern that the maintainers use.
However, do feel free to let me know if I should make separate PR contributions for some of these or if there are any changes within the commit messages that I should adjust. Thank you!!

[rhenium]

This is equivalent to if !omit_on_non_fips; return aws_lc? end which doesn't really make sense. I think you want return if aws_lc? (or return unless openssl?) on a separate line.

I'm not entirely sure if this test is useful to have in ruby/openssl's tests at all. OpenSSL's fips provider skipping generation and returning a pre-defined group sounds like an implementation detail to me. @junaruga What do you think?

[samuel40791765]

Sure, I'll do return unless openssl? for now. Agreed that this is probably a new implementation detail specific to OpenSSL FIPS, most libcrypto's normally don't deviate from the original behavior.

[rhenium]

I think we might be getting inconsistent results because the certificates are not conformant to RFC 5280. We should instead fix the certificates (@ca_cert and @svr_cert in utils.rb) so that they include SKI/AKI extensions.

     ca_exts = [
       ["basicConstraints","CA:TRUE",true],
       ["keyUsage","cRLSign,keyCertSign",true],
+      ["subjectKeyIdentifier","hash",false],
     ]
     ee_exts = [
       ["keyUsage","keyEncipherment,digitalSignature",true],
+      ["authorityKeyIdentifier","issuer:always,keyid:always",false],
     ]

[samuel40791765]

I gave this a shot, but this doesn't resolve the issue :(. Unfortunately, I think this has more to do with the fact that AWS-LC's certificate verification logic has not been updated to align with later versions of OpenSSL. We plan to make this a better experience in the near future, but completing the work will take time. I'll also update the commit message to mention certificate verification logic differences to make things clearer.
I found a similar issue with LibreSSL in the same test that was fixed a while ago: a9954ba. The same assertion originally existed for LibreSSL, so I've changed the assertion to match the original format.

That being said, I can add the certificate improvements within the same commit if you'd like us to? Although it does seem different from the certificate verification logic issues this is trying to work around though.

[rhenium]

You're right, it seems like a different issue.

I think the actual cause might be that SSL_MODE_NO_AUTO_CHAIN is turned on by default in AWS-LC, unlike OpenSSL: https://github.com/aws/aws-lc/blob/154f9986bc18f8f0e6126157a3abc84230456f77/include/openssl/ssl.h#L844-L861

ssl.peer_cert_chain on the client SSLSocket shows 2 certificates with OpenSSL and just 1 with AWS-LC, which explains the different error codes.

I don't think start_server should depend on this behavior, and it doesn't seem intentional according to what I can see in git log in ruby/ruby's tree. I'll make a separate PR to fix this.

[rhenium]

#858 removed the cert store from start_server which was triggering the different behavior.

test_verify_result and test_connect_certificate_verify_failed_exception_message should not need to relax the assertions.

[rhenium]

I think this description is not accurate. The input (pki_msg.data) uses indefinite length encodings, and OpenSSL re-encodes it into DER. AWS-LC's output is also DER, but the content is incorrect. If this is expected, I'd suggest skipping this test case for now.

Here is the inner PKCS#7 (pki_msg.data), formatted to PEM:

-----BEGIN PKCS7-----
MIAGCSqGSIb3DQEHA6CAMIACAQAxggEQMIIBDAIBADB1MHAxEDAOBgNVBAoMB2V4
YW1wbGUxFzAVBgNVBAMMDlRBUk1BQyBST09UIENBMSIwIAYJKoZIhvcNAQkBFhNz
b21lb25lQGV4YW1wbGUub3JnMQswCQYDVQQGEwJVUzESMBAGA1UEBwwJVG93biBI
YWxsAgFmMA0GCSqGSIb3DQEBAQUABIGAbKV17HvGYRtRRBNz1QLpW763UedhVj5K
Xi70o4BJGM04lItAgt6aFC9SruZjpWr1gCYKCaRSAg273DeGTQwsDoZ86CPXzBpp
tYLz0MteQXYYWUaPZT+xmvx4NgDyk9P9MoT7JifsPrtXuzqCRFXhGdu8d/ru+OWx
hHLvKH+bYekwgAYJKoZIhvcNAQcBMBQGCCqGSIb3DQMHBAgTbNlOZjLHf6CABIIC
EFOnLq/EAc9Nv+HjKR3ZVPSJMq0TImjGf5Mvc3nDgI572Hdo2aku0YXM6WjSWkpY
txpg7Cqxfl6hPSefLPUnBqlIoM2qbrE7MSKEVD6+2bW9GqYPFVg4qQLLsOxnxJIM
fOvLFfd7guL+iLH424XfiUUxaf8EdZE4u2IEl4REvkS1FoEGwyA4BEGMSeVPedQC
bZ0qY7Pc2tmZE3XfEUhIsyStG0Nb6i6AKcAFYGapbgE6kAB0gwsYcHlWMOvsvdAf
cTq6jwtHlO1s68qtvkWquTQ9lpX+fzddUUNxEHSqv5eU3oo6fT3Vj5ZFIVlaA5Th
ZMrI5PgRPuwJM4GL8/VLwY5mbDLFqn/irGeEvP99J3S87ornLLunjpxSy1/AymcV
ep2H32Tj82WS/IRQXBOzz4EnQRJGszKxAV6tY+Zje3sWyTTgObhlsiTQTDgnvtSW
8RvVHqKrwgkxxEsRHg7u8UdzZ0jg+O5+3F8B6/NWMyts0OaFqT9wvI8yO7VIy3dU
tGdz7Hde6Ggp/iTn1LbgdJ3N8Hzxf1j6NMWUKHVsadvwpRJbUeqq9c3+QuxsJi8w
WemxxQCE+tPyc1dP+ej5/M7bERbSOHMGgX03758IvP7A/fy2DjGPv2+lAwlEke0U
ze1367QKgxM0nc3SZDlptY7zPIJC5saWXb8Rt2bw2JxEBOTavrp+ZwJ8tcH961on
qwQIxOZ7YgJoLOQAAAAAAAAAAAAA
-----END PKCS7-----

Here is the EncryptedContentInfo part in the der2ascii output. It does show an indefinite length constructed encoding for EncryptedContent (OCTET STRING), which is implicit tagged [0].

      SEQUENCE indefinite {
        # data
        OBJECT_IDENTIFIER { 1.2.840.113549.1.7.1 }
        SEQUENCE {
          # DES-EDE3-CBC
          OBJECT_IDENTIFIER { 1.2.840.113549.3.7 }
          OCTET_STRING { `136cd94e6632c77f` }
        }
        [0] indefinite {
          OCTET_STRING { `53a72[...snip]` }
          OCTET_STRING { `c4e67b6202682ce4` }
        }
      }

In OpenSSL output (pki_message_content_pem), EncryptedContent uses an equivalent primitive encoding:

      SEQUENCE {
        # data
        OBJECT_IDENTIFIER { 1.2.840.113549.1.7.1 }
        SEQUENCE {
          # DES-EDE3-CBC
          OBJECT_IDENTIFIER { 1.2.840.113549.3.7 }
          OCTET_STRING { `136cd94e6632c77f` }
        }
        [0 PRIMITIVE] { `53a72[...snip...]2ce4` }
      }

In AWS-LC output (pki_message_content_pem_awslc), EncryptedContent also uses a primitive encoding, but the content contains extra OCTET STRING headers in it:

      SEQUENCE {
        # data
        OBJECT_IDENTIFIER { 1.2.840.113549.1.7.1 }
        SEQUENCE {
          # DES-EDE3-CBC
          OBJECT_IDENTIFIER { 1.2.840.113549.3.7 }
          OCTET_STRING { `136cd94e6632c77f` }
        }
        [0 PRIMITIVE] {
          OCTET_STRING { `53a72[...snip]` }
          OCTET_STRING { `c4e67b6202682ce4` }
        }
      }

I took a look at AWS-LC's d2i_PKCS7() and this part stood out to me: https://github.com/aws/aws-lc/blob/7518c784f4d6a8933345d9192b82ecc19bea4403/crypto/pkcs7/pkcs7_asn1.c#L45-L49

I haven't dug deeper yet, but CBS_asn1_ber_to_der() not taking any contextual information seems odd, as correct conversion of structures like this would require it.

[samuel40791765]

Great points and indeed there seems to be something we overlooked here. I'll sync up internally with our team to figure how to properly adjust this. Thanks!!

[samuel40791765]

I've done some deeper diving and I've pinned the reason down to AWS-LC's lack of support for ASN1_OCTET_STRING_NDEF and the ASN1_TFLG_NDEF flag. These act as a helper to request indefinite-length encoding for the ASN1 encoding functions in OpenSSL. This was removed from AWS-LC in this commit: aws/aws-lc@45858ae, and there's a mention of OpenSSL using this for their PKCS7 implementation within the commit message.

AWS-LC and OpenSSL both use the same generic ASN.1 functions which depend on the ASN.1 definition macros defined in each library. There's a slight difference in our definitions that I discovered here:

• https://github.com/aws/aws-lc/blob/154f9986bc18f8f0e6126157a3abc84230456f77/crypto/pkcs7/pkcs7_asn1.c#L149C5-L150
• https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/pkcs7/pk7_asn1.c#L148C51-L148C73

AWS-LC doesn't support the aforementioned ASN1_OCTET_STRING_NDEF tag that OpenSSL uses for PKCS7 and we decided to explicitly tag the type as ASN1_OCTET_STRING instead. This helps explain the additional OCTET STRING headers you're seeing with the translated ASN1 contents for AWS-LC.

I was looking through RFC2315's definition of EncryptedContentInfo to double check and it does explicitly call out that the EncryptedContent field should be an OCTET STRING. It's also mentioned later on in section 10.3 that:

Definite-length encoding is more appropriate for simple types such as octet strings, so definite-length encoding is chosen.

Despite the slight difference, we think both OpenSSL and AWS-LC have valid ASN1 representations, just one has the explicit OCTET STRING header, while OpenSSL doesn't.

I really appreciate the help with this. I'll patch the test to be skipped with AWS-LC for the time being and document the actual root cause more accurately.

[rhenium]

Despite the slight difference, we think both OpenSSL and AWS-LC have valid ASN1 representations, just one has the explicit OCTET STRING header, while OpenSSL doesn't.

I'm afraid you are mixing something up here. Both outputs are well formed DER (then formatted to PEM), but they are representations of different data. The contents octets of EncryptedContent in AWS-LC's start with 04 82 02 10 53 a7 ... instead of the correct 53 a7 ....

Ideally AWS-LC should either produce the identical output as OpenSSL, since DER is deterministic, or make d2i_PKCS7{,_bio}() return an error if such an input is not supported.

Anyway, skipping it in ruby/openssl's test suite seems reasonable for now.

[rhenium]

Reverting aws/aws-lc@2a72226 makes d2i_PKCS7{,_bio}() report an error when it encounters the constructed encoding of octet string (EncryptedContent).

I think reintroducing support for parsing constructed encoding of strings might require reverting this commit (perhaps excluding the part related to indefinite lengths): aws/aws-lc@a70edd4

[samuel40791765]

Thanks the review! We're still discussing a path forward on the PKCS7 comment/issue, but I've attempted to address the rest of your comments.

[samuel40791765]

Sure, I'll do return unless openssl? for now. Agreed that this is probably a new implementation detail specific to OpenSSL FIPS, most libcrypto's normally don't deviate from the original behavior.

[samuel40791765]

I gave this a shot, but this doesn't resolve the issue :(. Unfortunately, I think this has more to do with the fact that AWS-LC's certificate verification logic has not been updated to align with later versions of OpenSSL. We plan to make this a better experience in the near future, but completing the work will take time. I'll also update the commit message to mention certificate verification logic differences to make things clearer.
I found a similar issue with LibreSSL in the same test that was fixed a while ago: a9954ba. The same assertion originally existed for LibreSSL, so I've changed the assertion to match the original format.

That being said, I can add the certificate improvements within the same commit if you'd like us to? Although it does seem different from the certificate verification logic issues this is trying to work around though.

[samuel40791765]

Great points and indeed there seems to be something we overlooked here. I'll sync up internally with our team to figure how to properly adjust this. Thanks!!

[rhenium]

Sorry, my earlier suggestion wasn't right as the rescue clause would incorrectly catch an assertion failure from assert_equal (which utilizes exceptions). I think something like this should work:

Suggested change

	def assert_sign_verify_false_or_error
	begin
	assert_equal(false, yield)
	rescue => e
	assert_kind_of(OpenSSL::PKey::PKeyError, e)
	end
	end
	def assert_sign_verify_false_or_error
	ret = yield
	rescue => e
	assert_kind_of(OpenSSL::PKey::PKeyError, e)
	else
	assert_equal(false, ret)
	end