Investigate SAML Request signing

[joeniquette]

From IAM:

Based on the update of ISVA (formerly ISAM) to 10.0.3.1 last week in iDev we initially got reports of errors for both dev.va.gov and va.gov localhost based on the fact that the SAML AuthN requests are not signed. We were able to change a configuration to allow the unsigned authentication requests but as we discussed we need to look at ensuring VA.gov AuthN requests are signed.

• Task: Investigate why SAML request signing isn't working even though it does appear to be set to true in the vets-api repo and the vets-api configs in the devops repo

[joeniquette]

• lib/saml/ssoe_settings_service.rb

[joeniquette]

The signature would look like: 2:34 <?xml version="1.0" encoding="UTF-8"?>

https://ssoe.internal.sp.va.gov YIle4eaKKqModtDhv8TxF0tbLGY= jnGJdzA9/b9bbGZl26z/tW/kL2jLvPuv9iYoEAV7WXBaSImSpnYCsQJv7iqSHJDY0ymTZpT9UkTNmX1EHkvvcT5DurUSV1VnC2MAoKru8j5LoEokSDeIN54W8EEja4KREiRM+sEToET/rzV9zCB2lkkWIJpkG4V0amyeCFgTkXr11eL9U3xZbVURkRXEhCKX1icjCQVN9A0yYKXOfFedwoWTtGYp+TtPz4LBVlZ4l3DxRVcS7ceXGmrOsRGKbKPax6j5yOZoybzjFG7/QflWKp4LwSIZMDz0j/FdN1qi6i8CzuPwHaeNLgqJijvneTD4/lrTcrPd4QkTA2YIeokJYQ== MIIDUjCCAjqgAwIBAgIEUOLIQTANBgkqhkiG9w0BAQUFADBrMQswCQYDVQQGEwJGSTEQMA4GA1UE CBMHVXVzaW1hYTERMA8GA1UEBxMISGVsc2lua2kxGDAWBgNVBAoTD1JNNSBTb2Z0d2FyZSBPeTEM MAoGA1UECwwDUiZEMQ8wDQYDVQQDEwZhcG9sbG8wHhcNMTMwMTAxMTEyODAxWhcNMjIxMjMwMTEy ODAxWjBrMQswCQYDVQQGEwJGSTEQMA4GA1UECBMHVXVzaW1hYTERMA8GA1UEBxMISGVsc2lua2kx GDAWBgNVBAoTD1JNNSBTb2Z0d2FyZSBPeTEMMAoGA1UECwwDUiZEMQ8wDQYDVQQDEwZhcG9sbG8w ggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCXqP0wqL2Ai1haeTj0alwsLafhrDtUt00E 5xc7kdD7PISRA270ZmpYMB4W24Uk2QkuwaBp6dI/yRdUvPfOT45YZrqIxMe2451PAQWtEKWF5Z13 F0J4/lB71TtrzyH94RnqSHXFfvRN8EY/rzuEzrpZrHdtNs9LRyLqcRTXMMO4z7QghBuxh3K5gu7K qxpHx6No83WNZj4B3gvWLRWv05nbXh/F9YMeQClTX1iBNAhLQxWhwXMKB4u1iPQ/KSaal3R26pON UUmu1qVtU1quQozSTPD8HvsDqGG19v2+/N3uf5dRYtvEPfwXN3wIY+/R93vBA6lnl5nTctZIRsyg 0Gv5AgMBAAEwDQYJKoZIhvcNAQEFBQADggEBAFQwAAYUjso1VwjDc2kypK/RRcB8bMAUUIG0hLGL 82IvnKouGixGqAcULwQKIvTs6uGmlgbSG6Gn5ROb2mlBztXqQ49zRvi5qWNRttir6eyqwRFGOM6A 8rxj3Jhxi2Vb/MJn7XzeVHHLzA1sV5hwl/2PLnaL2h9WyG9QwBbwtmkMEqUt/dgixKb1Rvby/tBu RogWgPONNSACiW+Z5o8UdAOqNMZQozD/i1gOjBXoF0F5OksjQN7xoQZLj9xXefxCFQ69FPcFDeEW bHwSoBy5hLPNALaEUoa5zPDwlixwRjFQTc5XXaRpgIjy/2gsL8+Y5QRhyXnLqgO67BlLYW/GuHE= http://idmanagement.gov/ns/assurance/ial/2 http://idmanagement.gov/ns/assurance/aal/2

[bramleyjl]

From reading the OneLogin documentation in order to sign SAML messages we must first designate a certificate & a private_key in the settings object, before enabling the use of SAML signing for specific messages, which we do by setting authn_requests_signed to true.

I can confirm that authn_requests_signed is properly set in our devops repository, but as far as I can tell we are not including the certificate or key. The handling for those two settings exists in lib/saml/ssoe_settings_service.rb, but so far I can't find the existence of those values in the devops repo.

Update: I found in config/initializers/saml.rb that those settings are generated based off of the saml_ssoe.key_path & saml_ssoe.cert_path settings, both of which show up in devops configs

[bramleyjl]

The issue appears to be a mismatch between the default settings.idp_sso_service_binding, which is set to "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Artifact" and the RubySAML gem's requirement for SAML request signing, "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST". This mismatch can be found in the RubySAML gem @ vendor/bundle/ruby/2.7.0/gems/ruby-saml-1.14.0/lib/onelogin/ruby-saml/authrequest.rb

After manually setting the request service binding to POST the authentication request was signed, but was rejected by ISAM as the signature was not able to validated. My assumption is that the IAM team will have to change their ISAM configuration to be set up to decrypt the SAML payload with vet-api's public key.

[joeniquette]

IAM stated this should be safe to test in Dev and staging if we want to give the change a go.

[bramleyjl]

Related devops PR: https://github.com/department-of-veterans-affairs/devops/pull/11894

I'm also researching the difference betwen the "Artifact" service binding that we have now and the "POST" binding that this is changing our requests to in order to enable SAML signing. Is this something that we need to take into account?

[joeniquette]

Yes, we likely cannot just switch from artifact to post. In reading the document you posted there are some mentions of signing requests, and also of combining bindings to allow for both artifact and post. From the doc, it appears as though signing is used/required because they dont encrypt the contents. Whereas Artifact doesnt require signing because they expect encryption. Nothing in there states we cant do signing and encrypted responses with POST.

For not, review the documentation, if you dont find any concerns with using POST then lets give it a try.

[bramleyjl]

VSP Infra Application Manifests PR

[bramleyjl]

Further inquiry has revealed that the SAML message is unable to be signed due to a bug in the OneLogin RubySAML gem that vets-api leverages for its SAML communications, the compute_digest method that encrypts the Digest value:

    def compute_digest(document, digest_algorithm)
      digest = digest_algorithm.digest(document)
      Base64.encode64(digest).strip!
    end

The document param is the unencrypted SAML to be transformed, the digest_algorithm is controlled via a config, in this case it's OpenSSL::Digest::SHA256. After the digest is created in the first line, the RubySAML gem attempts to strip the result of leading & trailing whitespaces, but by using strip! instead of strip the method returns nil because no whitespaces were found to be removed. I have attempted altering the document contents by introducing leading & trailing whitespaces but the digest process always removes them so that they are not present when strip! is being called.

The long-term solution for this is for an update to the RubySAML, however the project is no longer under active maintenance as of approximately a month ago, which makes this seem unlikely. In lieu of that, our best course of action might be to fork our own version of the gem to make the bugfix ourselves (while also submitting a ticket to the project requesting it be merged into the original version).

[bramleyjl]

This is rough draft for an issue ticket on the RubySAML gem repository, any feedback @bosawt @joeniquette?

---

We are updating our SAML authentication requests to our service provider by including SAML signing, however the resulting DigestValue is always blank. I traced the problem back to the compute_digest method, which is using strip! to remove preceding and trailing whitespaces.

    def compute_digest(document, digest_algorithm)
      digest = digest_algorithm.digest(document)
      Base64.encode64(digest).strip!
    end

We are using OpenSSL::Digest::SHA256 as our digest_algorithm, but the Base64 encoding of the digest created on the previous line doesn't contain any preceding or trailing whitespaces, causing the strip! method to return nil instead of the supplied value.

What is the purpose of using strip! over the base strip method? Is it possible for it to be replaced with strip in this instance?

[bosawt]

This is rough draft for an issue ticket on the RubySAML gem repository, any feedback @bosawt @joeniquette?

We are updating our SAML authentication requests to our service provider by including SAML signing, however the resulting DigestValue is always blank. I traced the problem back to the compute_digest method, which is using strip! to remove preceding and trailing whitespaces.

    def compute_digest(document, digest_algorithm)
      digest = digest_algorithm.digest(document)
      Base64.encode64(digest).strip!
    end

We are using OpenSSL::Digest::SHA256 as our digest_algorithm, but the Base64 encoding of the digest created on the previous line doesn't contain any preceding or trailing whitespaces, causing the strip! method to return nil instead of the supplied value.

What is the purpose of using strip! over the base strip method? Is it possible for it to be replaced with strip in this instance?

Yeah, seems good to me

[bramleyjl]

After further effort I was unable to introduce a change to our SAML metadata that would induce a whitespace in the string that is passed into the strip! method, I've raised an issue ticket on the ruby-saml repository and am marking this as Blocked for now.

[bramleyjl]

SAML signing has been enabled in dev and staging, with a planned prod release @ 9 PM ET on 10/20

The decision was made to not enable SAML signing for localhost as it would have necessitated either including the signing private key into version control or requiring all local vets-api builds to download/generate a key to make SAML authentication work. ISAM will continue to accept signed or unsigned SAML requests from localhost builds.

[bramleyjl]

VSP Identity & the IAM team have confirmed that vets-api production is now signing SAML requests - the ISAM production switch to only accepting signed SAML requests is still due to occur at 9 EST tonight.

[bramleyjl]

SAML request signing is confirmed present & working VA.gov production.