Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WFLY-19269] [Preview] OCSP stapling support #564

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PrarthonaPaul
Copy link

@PrarthonaPaul PrarthonaPaul commented Apr 23, 2024

@PrarthonaPaul PrarthonaPaul requested a review from fjuma April 23, 2024 16:56
Copy link
Contributor

@fjuma fjuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @PrarthonaPaul! I've added some comments.

# Add any category for this proposal
# if missing, add it to _data/widfly-categories and use its id
---
= [Experimental|Preview|Community|default]Template <INSERT TITLE HERE>
Copy link
Contributor

@fjuma fjuma Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can go with Preview for now. The title here should be updated too along with the PR title.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

:idseparator: -

== Overview
Online Certificate Status Protocol (OCSP) is one of the methods of checking whether or not a certificate presented by a client is valid or not. It is a protocol for determining the status of a certificate and is described in http://www.ietf.org/rfc/rfc2560.txt[RFC 2560].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/certificate presented by a client is valid or not/certificate is valid or not

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!


=== Issue

* https://issues.redhat.com/browse/WFCORE[WFCORE-XXXX]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this will involve updates to the Elytron subsystem, we'll need a WFCORE issue to be created and linked here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the WF-Core issue.

== Requirements

=== Hard Requirements
* Existing OCSP support for WildFly's Elytron subsystem would be extended to allow for OCSP stapling. A new attribute, called `enable-stapling`, would be added to `elytron/key-manager` to allow for stapling support.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing OCSP support is part of the trust-manager configuration. For that, we are configuring details about the OCSP responder that the server will use when trying to determine if the certificate presented by the client has been revoked.

However, OCSP stapling is something that will be used by the server to staple the response from the OCSP responder to its own certificate that it will present to the client. So this new configuration should be separate from the server's trust-manager configuration.

This line mentions key-manager but the sample configuration below shows the trust-manager so maybe it's just the sample config below that needs to be updated?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the server-side proposals to add to the key-manager resource instead of the trust-manager.


** This attribute would be of type boolean. To allow for backwards compatibility, this attribute's value would be false by default, which would allow for traditional OCSP support.

Current Subsystem Configuration:
Copy link
Contributor

@fjuma fjuma Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/ocsp.html#ocsp-stapling-and-certificate-revocation.

This explains OCSP stapling and system properties that can be used to enable it/configure it. This will be helpful to determine the types of things that could be added to the configuration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a WildFly server is acting like a client (e.g., via client-ssl-context configuration), it should be able to make use of the OCSP response stapled to the certificate returned by the server its communicating with.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the new attributes based on the protocol information in the link above. Thanks for sharing it.

=== Hard Requirements
* Existing OCSP support for WildFly's Elytron subsystem would be extended to allow for OCSP stapling. A new attribute, called `enable-stapling`, would be added to `elytron/key-manager` to allow for stapling support.

** This attribute would be of type boolean. To allow for backwards compatibility, this attribute's value would be false by default, which would allow for traditional OCSP support.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing OCSP support is for the server's trust-manager so this would be separate from OCSP stapling.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

<trust-store key-store-name="ca"/>
<trust-manager algorithm="PKIX" maximum-cert-path=4 soft-fail="true" only-leaf-cert="true"/>
<certificate-revocation-list path="crl.pem"/>
<ocsp responder="responderUri" enable-stapling=true responder-certificate="certalias" responder-keystore="responder-store"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the client configuration, the client needs to be able to make use of the OCSP response stapled to the certificate that's being presented by the server.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New client config is TBD.
I will look more into what needs to be defined and will update client side proposal.

# Add any category for this proposal
# if missing, add it to _data/widfly-categories and use its id
---
= [Preview] OCSP Stapling Support
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the PR title as well, thanks!

** `cacheLifetime` which controls the maximum lifetime of a cached response in seconds. The default value is 3600 s or 1 hour.
** `responderURI` which is the responder to contact in case the certificate used by the server does not have the `Authority Info Access` (AIA) extension. This does not override the AIA extension value unless `responderOverride` is set to true. The default value for this is undefined.
** `responderOverride` which determines whether or not the Authority information from the AIA extension value would be overridden by the value of the `responderURI`. The default value for this is false.
** `ignoreExtensions` which determines whether the forwarding of OCSP extensions specified in the `status_request` or `status_request_v2` TLS extensions is disabled or not. The default value for this is false.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A - is being used in some attribute names but not all. We should be consistent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to indicate if any of these attributes are required or if they are all optional.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated all attributes to use - and all lowercase.

== Requirements

=== Hard Requirements
* Existing OCSP support for WildFly's Elytron subsystem would be extended to allow for OCSP stapling. A new attribute, called `ocsp-stapling`, would be added to `elytron/key-manager` to allow for stapling support. Additionally, this attribute would have the following attributes nested under it:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a new element with attributes (as opposed to "attribute would have the following attributes under it").

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments!
This resource (https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/ocsp.html#ocsp-stapling-and-certificate-revocation) has the following blurb under Server-Side Properties.
"Most of the properties are read at SSLContext instantiation time. This means that if you set a property, you must obtain a new SSLContext object so that an SSLSocket or SSLEngine object you obtain from that SSLContext object will reflect the property setting. The one exception is the jdk.tls.stapling.responseTimeout property. That property is evaluated when the ServerHandshaker object is created (essentially at the same time that an SSLSocket or SSLEngine object gets created)."

This makes me think, would the new element and attributes be better added to SSL-context instead of key-manager?
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would make sense.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, awesome. Thanks!
I have added these attributes under server-ssl-context and updated the proposed configuration

@PrarthonaPaul PrarthonaPaul changed the title [WFLY-19269] OCSP stapling support [WFLY-19269] [Preview] OCSP stapling support Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants