Enable OCSP revocation check for trusted CA #3593

kyberpunk · 2023-12-09T12:15:30Z

Hello, based on issue #3588 I implemented OCSP revocation check support when using trusted CAs for device authentication in tenant.

PR contains following functionality:

Enabling OCSP revocation check on configured trusted CA in tenant
Configuring useful OCSP settings per CA - OCSP URI, OCSP CA, nonce, check end entities only
Tried to keep and public methods and network methods stable where possible.

To reconsider:

Implemented OCSP settings on trusted CA level for the best flexibility. It covers also cases when user needs to setup e.g. two CAs with different OCSP URLs. However, it implied more changes in code. Alternatively we could do it just on tenant level or using Java properties. But this will affect flexibility in cases there will be different CAs with different revocation options used at the same time.
Instead of extending TrustAnchor can use new data class, but not sure if this API can be used by someone outside of Hono code.

I'm looking forward to your feedback what would you change. Once you confirm that code changes are ok, I will can also extend tests and documentation. Currently the changes were tested just manually.

closes #3588

...se/src/main/java/org/eclipse/hono/service/management/tenant/TrustedCertificateAuthority.java

core/src/main/java/org/eclipse/hono/util/TenantObject.java

kyberpunk · 2024-01-04T09:22:30Z

@sophokles73 I was also thinking about integration tests - we are using OpenSSL OCSP server functionality for testing, so I can imagine using Testcontainers to setup OCSP responder for JUnit tests. Do you think it is good idea to do it this way?

kyberpunk · 2024-01-09T13:27:23Z

Hi @sophokles73 , I've incorporated your remarks and implemented comprehensive unit and integration tests. Can you please check it?

There is currently just one remaining issue related to the decision of using just subject and public key for trust anchor. OCSP request defines issuerNameHash which is created from DER encoding of CA issuer name. We are storing subject just as string and by TrustAnchor it is interpreted internally as X500 name with UTF8String type, but in original certificate it can be (and usually is) PrintableString type. This leads to inconsistent hashes with original certificate. This can lead to incompatibility with some responders (e.g. OpenSSL implementation does not work properly).

Can I add new field in TrustedCertificateAuthority for storing also the binary form of subject (similarly as pub key)? This would ensure correct compatibility according to specification.

sophokles73 · 2024-01-11T06:53:19Z

I hope to find the time to look into this more deeply next week. Sorry for the delay ...

kyberpunk · 2024-01-14T12:50:01Z

I've implemented also storing of DER encoded subject DN as mentioned, so now OCSP should work properly in most scenarios.

kyberpunk · 2024-01-19T08:51:24Z

Hi @sophokles73 , did you please have a chance to check it?

sophokles73 · 2024-01-22T07:27:34Z

not yet, sorry for the delay

adapter-base/src/main/java/org/eclipse/hono/adapter/auth/device/x509/OCSPNonceExtension.java

...ter-base/src/test/java/org/eclipse/hono/adapter/auth/device/x509/OCSPNonceExtensionTest.java

site/documentation/content/api/management/device-registry-v1.yaml

core/src/main/java/org/eclipse/hono/util/RevocableTrustAnchor.java

sophokles73 · 2024-02-14T07:51:12Z

adapter-base/src/test/resources/ocsp/cacert.pem

@@ -0,0 +1,12 @@
+-----BEGIN CERTIFICATE-----
+MIIBtTCCATugAwIBAgIBADAKBggqhkjOPQQDBDAaMRgwFgYDVQQDDA9pb3QuZWNs


will any of the tests fail if any of the ceriticates' validity period has expired?

Yes it will, but validity of new certificates is set to 50 years so I hope it is sufficient countermeasure.

adapter-base/src/test/java/org/eclipse/hono/adapter/auth/device/x509/OCSPIntegrationTest.java

...ter-base/src/test/java/org/eclipse/hono/adapter/auth/device/x509/OCSPNonceExtensionTest.java

adapter-base/pom.xml

Signed-off-by: Vit Holasek <[email protected]>

…sted CA. Signed-off-by: Vit Holasek <[email protected]>

Signed-off-by: Vit Holasek <[email protected]>

…ved methods for RevocableTrustAnchor setup. Signed-off-by: Vit Holasek <[email protected]>

Signed-off-by: Vit Holasek <[email protected]>

… inconsistent issuerNameHash in OCSP Signed-off-by: Vit Holasek <[email protected]>

Signed-off-by: Vit Holasek <[email protected]>

adapter-base/pom.xml

sophokles73 · 2024-02-26T07:18:20Z

...se/src/main/java/org/eclipse/hono/service/management/tenant/TrustedCertificateAuthority.java

+     * Gets OCSP responder certificate which is used to verify OCSP response signature. If custom certificate is not
+     * set then issuing CA of client certificate is used instead.


IMHO this will only work, if the user also includes the trust anchor certificate in its request, because otherwise cert will always be null as we do not store the cert in the DB but only its public key and subject DN.
This means that we can not later enable OCSP using the trust anchor CA for this purpose on an already existing trust anchor, or am I mistaken?

Technically yes. Since OCSP implementation requires full certificate, user must provide it either in cert or ocsp-responder-cert property. I may test what happens in such case, when you just enable it by PUT, but it should be handled by validation.

site/documentation/content/api/management/device-registry-v1.yaml

sophokles73 · 2024-02-26T07:21:05Z

site/documentation/content/api/management/device-registry-v1.yaml

+                  X.509 certificate. If the `cert` property is used to provide
+                  an X.509 certificate then the subject DN is determined from
+                  the certificate and this property is ignored.
+                  Otherwise this property is optional, but may be needed to


may be needed is very unspecific. Please be clear about when this property is required.

Improved description

sophokles73 · 2024-02-26T07:22:29Z

site/documentation/content/api/management/device-registry-v1.yaml

+               type: string
+               description: |
+                  Specifies the OCSP responder uri which will be used for OCSP revocation check of this trust
+                  anchor. If URI is not set then certificate AIA extension value is used to obtain the values


... the AIA extension value of the certificate to check is used ...

sophokles73 · 2024-02-26T07:25:13Z

adapter-base/src/test/resources/ocsp/cakey.pem

@@ -0,0 +1,6 @@
+-----BEGIN EC PRIVATE KEY-----


I am not happy with having these certificates in the code base which seem to have happened to just come out of nowhere.
Can you please include a script for creating them in the demo-certs folder or extend the existing script and then use the keys from the demo-certs artifact as we do in other modules?

Ok, added second device certificate to demo script and now we fetch certificates and information from that package instead of extra static files. It just required small fix, to mark certificates as end entity by CA:FALSE in constrains, otherwise the Java implementations ignores them.

Signed-off-by: Vit Holasek <[email protected]>

…tantiation. Signed-off-by: Vit Holasek <[email protected]>

Signed-off-by: Vit Holasek <[email protected]>

kyberpunk · 2024-03-04T11:59:44Z

@sophokles73 I described the OCSP revocation check experimental feature in the documentation as you requested on open call.

sophokles73

Just two small textual changes and we are good to merge :-)

sophokles73 · 2024-03-05T06:43:57Z

site/documentation/content/concepts/device-identity.md

+response is then used to verify the client certificate's revocation status. Revocation check can be configured using the 
+[Tenant](/hono/docs/api/management/#/tenants) resource of Device Registry Management API.
+
+OCSP revocation check is supported for HTTP, MQTT, AMQP and CoAP protocol adapters.


I guess it would be fair to say that it is supported with all protocol adapters, based on the way you have integrated it ...

sophokles73 · 2024-03-05T06:44:27Z

site/documentation/content/concepts/device-identity.md

+OCSP revocation check is supported for HTTP, MQTT, AMQP and CoAP protocol adapters.
+
+{{% notice info %}}
+This feature is experimental and may be subject to change in future releases. Current implementation checks the 


... in future releases without further notice.

Signed-off-by: Vit Holasek <[email protected]>

sophokles73

LGTM

Thanks for contributing and your patience. IMHO this is a great outcome 👍

kyberpunk requested review from kaniyan, dejanb and sophokles73 as code owners December 9, 2023 12:15

kyberpunk force-pushed the enable_revocation_check_for_trusted_ca branch 2 times, most recently from 4072e09 to 73472a4 Compare December 9, 2023 12:37

sophokles73 requested changes Jan 3, 2024

View reviewed changes

...se/src/main/java/org/eclipse/hono/service/management/tenant/TrustedCertificateAuthority.java Outdated Show resolved Hide resolved

core/src/main/java/org/eclipse/hono/util/TenantObject.java Outdated Show resolved Hide resolved

sophokles73 reviewed Jan 3, 2024

View reviewed changes

core/src/main/java/org/eclipse/hono/util/TenantObject.java Outdated Show resolved Hide resolved

kyberpunk requested a review from sophokles73 January 9, 2024 13:18

sophokles73 requested changes Feb 14, 2024

View reviewed changes

kyberpunk added 8 commits February 23, 2024 13:08

Enabled client certificate revocation check.

1c2e6c8

Signed-off-by: Vit Holasek <[email protected]>

Extracted method from long method.

cfe41fd

Signed-off-by: Vit Holasek <[email protected]>

Implemented possibility to configure OCSP revocation settings per tru…

e481924

…sted CA. Signed-off-by: Vit Holasek <[email protected]>

Removed unused properties.

c57c0f8

Signed-off-by: Vit Holasek <[email protected]>

Fixed commit issues.

c632087

Signed-off-by: Vit Holasek <[email protected]>

Foxed code review issues - create anchor from subject and pub key, mo…

77d882a

…ved methods for RevocableTrustAnchor setup. Signed-off-by: Vit Holasek <[email protected]>

Implemented unit and integration tests for OCSP revocation check.

0e28958

Signed-off-by: Vit Holasek <[email protected]>

Implemented storing of DER encoded CA subject DN, avoid problems with…

da368a0

… inconsistent issuerNameHash in OCSP Signed-off-by: Vit Holasek <[email protected]>

kyberpunk force-pushed the enable_revocation_check_for_trusted_ca branch from 9f49c91 to 44081c7 Compare February 24, 2024 11:06

Incorporated review comments

e16345b

Signed-off-by: Vit Holasek <[email protected]>

kyberpunk force-pushed the enable_revocation_check_for_trusted_ca branch from 44081c7 to e16345b Compare February 24, 2024 11:07

kyberpunk requested a review from sophokles73 February 25, 2024 12:52

sophokles73 requested changes Feb 26, 2024

View reviewed changes

Improved tests and fixed review comments

52fa2f0

Signed-off-by: Vit Holasek <[email protected]>

kyberpunk requested a review from sophokles73 February 27, 2024 12:38

kyberpunk added 3 commits February 27, 2024 14:42

Load certificates from file system in BeforeAll instead of static ins…

e6ea136

…tantiation. Signed-off-by: Vit Holasek <[email protected]>

Fixed unused import

ded3ffc

Signed-off-by: Vit Holasek <[email protected]>

Described OCSP revocation check in the documentation.

7915be3

Signed-off-by: Vit Holasek <[email protected]>

sophokles73 requested changes Mar 5, 2024

View reviewed changes

Updated documentation based on code review.

8dbb442

Signed-off-by: Vit Holasek <[email protected]>

kyberpunk requested a review from sophokles73 March 5, 2024 08:44

sophokles73 approved these changes Mar 6, 2024

View reviewed changes

sophokles73 added Feature Request A request for adding new functionality to Hono security Issues regarding system/data security/privacy labels Mar 6, 2024

sophokles73 merged commit 7dcf993 into eclipse-hono:master Mar 6, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable OCSP revocation check for trusted CA #3593

Enable OCSP revocation check for trusted CA #3593

kyberpunk commented Dec 9, 2023 •

edited

Loading

kyberpunk commented Jan 4, 2024

kyberpunk commented Jan 9, 2024

sophokles73 commented Jan 11, 2024

kyberpunk commented Jan 14, 2024

kyberpunk commented Jan 19, 2024

sophokles73 commented Jan 22, 2024

sophokles73 Feb 14, 2024

kyberpunk Feb 23, 2024 •

edited

Loading

sophokles73 Feb 26, 2024

kyberpunk Feb 26, 2024

sophokles73 Feb 26, 2024

kyberpunk Feb 27, 2024

sophokles73 Feb 26, 2024

kyberpunk Feb 27, 2024

sophokles73 Feb 26, 2024

kyberpunk Feb 27, 2024

kyberpunk commented Mar 4, 2024

sophokles73 left a comment

sophokles73 Mar 5, 2024

kyberpunk Mar 5, 2024

sophokles73 Mar 5, 2024

kyberpunk Mar 5, 2024

sophokles73 left a comment

		@@ -0,0 +1,12 @@
		-----BEGIN CERTIFICATE-----
		MIIBtTCCATugAwIBAgIBADAKBggqhkjOPQQDBDAaMRgwFgYDVQQDDA9pb3QuZWNs

		* Gets OCSP responder certificate which is used to verify OCSP response signature. If custom certificate is not
		* set then issuing CA of client certificate is used instead.

Enable OCSP revocation check for trusted CA #3593

Enable OCSP revocation check for trusted CA #3593

Conversation

kyberpunk commented Dec 9, 2023 • edited Loading

kyberpunk commented Jan 4, 2024

kyberpunk commented Jan 9, 2024

sophokles73 commented Jan 11, 2024

kyberpunk commented Jan 14, 2024

kyberpunk commented Jan 19, 2024

sophokles73 commented Jan 22, 2024

Choose a reason for hiding this comment

kyberpunk Feb 23, 2024 • edited Loading

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

kyberpunk commented Mar 4, 2024

sophokles73 left a 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

sophokles73 left a comment

Choose a reason for hiding this comment

kyberpunk commented Dec 9, 2023 •

edited

Loading

kyberpunk Feb 23, 2024 •

edited

Loading