FI-2912: Implement trust verification of authorization server certificates #1

alisawallace · 2024-07-30T18:59:25Z

Summary

This PR implements Inferno (as a mock UDAP client) establishing trust with the authorization server's X.509 certificates provided in the server's Discovery response, specifically the signed_metadata JWT element. This behavior is included in an additional, optional test and requires testers to provide one or more trust anchor certificates (i.e., root CAs) as test inputs.

Additional notes:

JWT signature verification and trust chain verification of the x5c header certificates are intentionally separated into two separate steps to better support unit testing and error debugging of both capabilities.
- As a result, this test kit includes two separate sets of test certificates:
  - EMR Direct test certificates, obtained from the Discovery metadata response from the reference implementation at https://stage.healthtogo.me:8181/fhir/r4/stage, used to test cert trust chain verification tests.
  - Inferno self-generated certificates, used for testing the generation and verification of UDAP JWTs (since we need access to a private key to make JWTs)
The UDAP JWT builder module now support multiple certificates being included in the x5c header. Dynamic Client registration tests and their inputs were updated to accept a comma separated list of one or more certs instead of a single cert
Input instructions and omitting the use of auto-generated certs prior to release will be handled in a separate PR

Testing Guidance

All new capabilities are thoroughly unit tested and all spec tests pass
Discovery test group can be tested against the reference implementation with the following values as inputs:
- FHIR Base Server URL: https://stage.healthtogo.me:8181/fhir/r4/stage
- Trust anchor inputs: provide the PEM text (separated by a comma) for both EMRDirectTestIntermediateCA.pem and EMRDirectTestRootCA.pem (in lib/udap_security/certs/testing)

Jammjammjamm · 2024-08-02T12:24:17Z

lib/udap_security/generate_client_certs_test.rb

@@ -13,8 +13,9 @@ class GenerateClientCertsTest < Inferno::Test
    input :udap_client_cert_pem,
          title: 'X.509 Client Certificate(s) (PEM Format)',
          description: %(
-            A client X.509 certificate in PEM format. It MUST be signed by a cert trusted by the authorization server
-            under test.
+            A comma-separted list of one or more X.509 certificates in PEM format. The first (leaf) certificate MUST


Is a comma typically used as a delimiter for certs?

As far as my research has indicated, there isn't really a standard delimiting character for certificate chains, rather when making a chain of certs (in PEM format) they are just appended like so:

---- BEGIN CERTIFICATE ---- ... ---- END CERTIFICATE ---- ---- BEGIN CERTIFICATE ---- ... ---- END CERTIFICATE ----

Ideally there would be an existing Ruby/OpenSSL library method that splits a cert chain PEM string into an array of certs, but the OpenSSL library only supports loading in from a file path (see this OpenSSL PR), which won't work for Inferno test inputs.

For time and simplicity's sake I went ahead with a comma delimiter. If there's a straightforward way of doing this without the comma let me know though, I'm not very familiar with Regexes or existing Ruby features that would make this easy to do.

ᐅ pry [1] pry(main)> pem_string = <<~PEM [1] pry(main)* ---- BEGIN CERTIFICATE ---- CERT1 ---- END CERTIFICATE ---- ---- BEGIN CERTIFICATE ---- CERT2 ---- END CERTIFICATE ---- [1] pry(main)* PEM => "---- BEGIN CERTIFICATE ----\nCERT1\n---- END CERTIFICATE ----\n---- BEGIN CERTIFICATE ----\nCERT2\n---- END CERTIFICATE ----\n" [2] pry(main)> regex = /(---- BEGIN CERTIFICATE ---- .*? ---- END CERTIFICATE ----)/ => /(---- BEGIN CERTIFICATE ---- .*? ---- END CERTIFICATE ----)/ [3] pry(main)> pem_string.scan(regex) => [["---- BEGIN CERTIFICATE ----\nCERT1\n---- END CERTIFICATE ----"], ["---- BEGIN CERTIFICATE ----\nCERT2\n---- END CERTIFICATE ----"]]

@Jammjammjamm I updated the code to use a regex instead of a comma, and refactored the splitting of user input to its own method for code reuse.

Jammjammjamm · 2024-08-02T12:28:23Z

lib/udap_security/signed_metadata_trust_verification_test.rb

+      assert token_header.key?('x5c'), 'JWT header does not contain `x5c` field'
+      assert token_header.key?('alg'), 'JWT header does not contain `alg` field'
+
+      anchor_certs_parsed = udap_server_trust_anchor_certs.split(',')


These certs don't seem parsed. You should probably get rid of this variable so that you don't have to come up with a name for it.

Yeah, misnomer there, I meant "split" rather than parsed but removed the variable like you suggested.

Jammjammjamm · 2024-08-02T12:33:44Z

lib/udap_security/udap_jwt_validator.rb

+        OpenSSL::X509::Certificate.new(cert_der)
+      end
+      crl_uris = cert_chain.map(&:crl_uris).compact
+      crl_uris = crl_uris.flatten


there's no reason to separate flatten from the rest of the method calls.

Leftover from a debugging session, fixed so it's all one line now!

Jammjammjamm · 2024-08-06T16:54:24Z

lib/udap_security/udap_jwt_validator.rb

+        # As a result, these capabilities are decoupled for testing purposes
+        JWT::X5cKeyFinder.new(trust_anchor_certs,
+                              crls).from(x5c_header_encoded)
+        [true, nil]


A hash is a much better way to return multiple values from a method than relying on array position to determine the meaning.

Good to know, I refactored both validate_trust_chain and validate_signature to return a hash with entries for success and error_message.

Jammjammjamm · 2024-08-06T16:55:49Z

lib/udap_security/certs/testing/EMRDirectTestClient.pem

@@ -0,0 +1,30 @@
+-----BEGIN CERTIFICATE-----


I don't feel good about having someone else's certs in our repo.

Yeah I totally get that, unfortunately until our self-generated certs are up to snuff, others' certs are all that are available for testing the trust chain validation method. The good-ish news is that, based on the way I broke this up the validate trust chain method does not require UDAP compliant certs, just any valid set of certificates.

Have you heard of there being publicly available test certificates for use cases like this? If we can find some we could include those instead. The alternative is to remove the trust chain validation unit tests (for both the module and the Inferno test) from the test kit since both use the EMR Direct certs. What are your thoughts?

Files which are only used for unit tests should live in spec/fixtures.

Good to know, I moved the EMR Direct certs to spec/fixtures

Jammjammjamm

I think this looks good, but unit tests and linting need to pass.

Jammjammjamm · 2024-08-09T14:30:05Z

spec/udap_security/signed_metadata_trust_verification_test_spec.rb

+      result = run(runnable, signed_metadata_jwt: create_test_jwt,
+                             udap_server_trust_anchor_certs: invalid_trust_anchor)
+      expect(result.result).to eq('fail')
+      expect(result.result_message).to match(self_signed_cert_error)


Running locally, I get:

Failures: 1) UDAPSecurity::SignedMetadataTrustVerificationTest when JWT includes client, intermediate, and root certs fails when incorrect root CA is provided as trust anchor Failure/Error: expect(result.result_message).to match(self_signed_cert_error) expected "Trust could not be established with server certificates, error message: /Users/smacvicar/.asdf/insta...icar/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-core-3.12.0/exe/rspec:4:in `<main>'\n" to match "self-signed certificate in certificate chain" Diff: @@ -1,62 +1,123 @@ -self-signed certificate in certificate chain +Trust could not be established with server certificates, error message: /Users/smacvicar/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/jwt-2.7.1/lib/jwt/x5c_key_finder.rb:30:in `from': Certificate verification failed: self signed certificate in certificate chain. Certificate subject: /C=US/ST=CA/L=San Diego/O=EMR Direct/OU=Certification Authority (certs.emrdirect.com)/CN=EMR Direct Test CA. (JWT::VerificationError) + from /Users/smacvicar/code/udap-test-kit/lib/udap_security/udap_jwt_validator.rb:45:in `validate_trust_chain' + from /Users/smacvicar/code/udap-test-kit/lib/udap_security/signed_metadata_trust_verification_test.rb:44:in `block in <class:SignedMetadataTrustVerificationTest>' + from /Users/smacvicar/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/inferno_core-0.4.20/lib/inferno/test_runner.rb:77:in `instance_eval' + from /Users/smacvicar/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/inferno_core-0.4.20/lib/inferno/test_runner.rb:77:in `run_test' + from /Users/smacvicar/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/inferno_core-0.4.20/lib/inferno/test_runner.rb:51:in `run' + from /Users/smacvicar/code/udap-test-kit/spec/udap_security/signed_metadata_trust_verification_test_spec.rb:65:in `run' + from /Users/smacvicar/code/udap-test-kit/spec/udap_security/signed_metadata_trust_verification_test_spec.rb:120:in `block (3 levels) in <top (required)>'

I see it passes on CI, though.

Odd, looks like semantically the error message is the same but on your machine it's self signed and on mine/CI it's self-signed. I updated the expected error messages to use a regex which should fix this.

…f ref server response

alisawallace · 2024-08-09T16:16:00Z

@Jammjammjamm I believe I fixed the source of the failing unit test but it seems to be machine-dependent (I had no errors with it initially or now), so maybe verify it's also passing on your machine.

Also, I fixed all the linting errors locally and rubocop runs clean for me, but on CI I'm seeing the following error that I don't know how to fix:

Run bundle exec rubocop
vendor/bundle/ruby/3.1.0/gems/fhir_client-5.0.3/.rubocop_todo.yml: Metrics/LineLength has the wrong namespace - replace it with Layout/LineLength
Error: `Performance/*` has been extracted to the `rubocop-performance` gem.
(obsolete configuration found in vendor/bundle/ruby/3.1.0/gems/fhir_client-[5](https://github.com/inferno-framework/udap-test-kit/actions/runs/10322291366/job/28577196311?pr=1#step:4:6).0.3/.rubocop_todo.yml, please update it)
Error: Process completed with exit code 2.

alisawallace added 3 commits July 30, 2024 10:10

Server trust verification implementation

6b5b67d

Minor fixes, rubocop fixes

335f295

Support multiple certs as client registration input

b930ed8

alisawallace requested a review from Jammjammjamm July 30, 2024 19:41

alisawallace added 3 commits August 1, 2024 15:00

Removed debugger break point

bd33338

Actually removing debugger references

61f455b

Skip trust verification test if no trust anchor input

cb27871

Jammjammjamm reviewed Aug 6, 2024

View reviewed changes

alisawallace added 4 commits August 6, 2024 11:02

Small method call fixes

bec93e8

Refactor validate_trust_chain to return hash instead of array

99170a5

Refactor validate_signature to return hash instead of array

cc15d74

Removed redundant method call

1e2b19a

alisawallace requested a review from Jammjammjamm August 6, 2024 18:45

alisawallace and others added 6 commits August 7, 2024 08:42

Refactor to use input parsing method

f1e534d

Refactor name of cert input processing method

0741209

Refactor split user input certs to use regex instead of comma delimiter

8acdb00

Use cert splitting method for processing trust anchor inputs

dc893aa

Updated test input descriptions

ac292d2

add ci

b0b532d

Jammjammjamm reviewed Aug 9, 2024

View reviewed changes

alisawallace added 4 commits August 9, 2024 09:09

Move unit test certs to spec/fixtures directory

39462a8

Change expected error message match from string literal to regex

992adda

Rubocop fixes

6657645

Updated signed_metdata_contents spec test to use custom JWT instead o…

f314617

…f ref server response

alisawallace requested a review from Jammjammjamm August 9, 2024 16:16

remove vendor from rubocop

0e5951b

Jammjammjamm changed the title ~~Implement trust verification of authorization server certificates~~ FI-2912: Implement trust verification of authorization server certificates Aug 9, 2024

Jammjammjamm approved these changes Aug 9, 2024

View reviewed changes

alisawallace merged commit 7735850 into main Aug 9, 2024
3 checks passed

Jammjammjamm deleted the FI-2912-server-cert-trust branch August 12, 2024 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FI-2912: Implement trust verification of authorization server certificates #1

FI-2912: Implement trust verification of authorization server certificates #1

alisawallace commented Jul 30, 2024

Jammjammjamm Aug 2, 2024

alisawallace Aug 6, 2024

Jammjammjamm Aug 7, 2024

alisawallace Aug 7, 2024

Jammjammjamm Aug 2, 2024

alisawallace Aug 6, 2024

Jammjammjamm Aug 2, 2024

alisawallace Aug 6, 2024

Jammjammjamm Aug 6, 2024

alisawallace Aug 6, 2024

Jammjammjamm Aug 6, 2024

alisawallace Aug 6, 2024

Jammjammjamm Aug 8, 2024

alisawallace Aug 8, 2024

Jammjammjamm left a comment

Jammjammjamm Aug 9, 2024

alisawallace Aug 9, 2024

alisawallace commented Aug 9, 2024

FI-2912: Implement trust verification of authorization server certificates #1

FI-2912: Implement trust verification of authorization server certificates #1

Conversation

alisawallace commented Jul 30, 2024

Summary

Testing Guidance

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jammjammjamm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisawallace commented Aug 9, 2024