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

Add SslInfoContributor and SslHealthIndicator #41205

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

Conversation

jonatan-ivanov
Copy link
Member

@jonatan-ivanov jonatan-ivanov commented Jun 21, 2024

Production incidents because of invalid certificates are common issues in the industry. SslInfoContributor and SslHealthIndicator in this PR can help to mitigate them, they:

  • Provide extra information about the used certificates (issuer, subject, validity, etc.)
  • Indicate if a certificate is invalid and the reason of it
  • Indicate if a certificate will be invalid within a configurable threshold (e.g.: in a week)

This PR is a draft/proof of concept right now (to gather feedback), it does not have tests (has a temporary(?) demo using spring-boot-smoke-test-tomcat-ssl) nor docs but it has quite a few TODOs (see them in the comments):

  • The certificate validity warning threshold is hardcoded right now
  • Some configuration (bean creation) could be simplified
  • Health Status could be enhanced by adding a new status: WARNING
  • WebServerSslBundle.get call could be optimized and its result reused
  • I haven't checked what happens if the cert is reloaded

Example /info and /health outputs:

/info of a VALID cert (click here to expand)
{
  "ssl": {
    "bundles": [
      {
        "name": "ssldemo",
        "certificateChains": [
          {
            "alias": "spring-boot",
            "certificates": [
              {
                "version": "V3",
                "signatureAlgorithmName": "SHA256withRSA",
                "validityStarts": "2024-06-21T21:17:02Z",
                "validityEnds": "2024-07-05T21:17:02Z",
                "validity": {
                  "status": "VALID"
                },
                "serialNumber": "9fbcacfed83af328",
                "issuer": "CN=localhost,OU=Spring,O=VMware,L=Palo Alto,ST=California,C=US",
                "subject": "CN=localhost,OU=Spring,O=VMware,L=Palo Alto,ST=California,C=US"
              }
            ]
          }
        ]
      }
    ]
  }
}
/health of a VALID cert (click here to expand)
{
  "status": "UP",
  "components": {
    "ping": {
      "status": "UP"
    },
    "ssl": {
      "status": "UP"
    }
  }
}
/info of an EXPIRED cert (click here to expand)
{
  "ssl": {
    "bundles": [
      {
        "name": "ssldemo",
        "certificateChains": [
          {
            "alias": "spring-boot-ssl-sample",
            "certificates": [
              {
                "version": "V3",
                "validity": {
                  "status": "EXPIRED",
                  "message": "Not valid after 2014-10-21T19:48:43Z"
                },
                "validityStarts": "2014-07-23T19:48:43Z",
                "validityEnds": "2014-10-21T19:48:43Z",
                "signatureAlgorithmName": "SHA256withRSA",
                "serialNumber": "7207ee6e",
                "issuer": "CN=localhost,OU=Unknown,O=Unknown,L=Unknown,ST=Unknown,C=Unknown",
                "subject": "CN=localhost,OU=Unknown,O=Unknown,L=Unknown,ST=Unknown,C=Unknown"
              }
            ]
          }
        ]
      }
    ]
  }
}
/health of an EXPIRED cert (click here to expand)
{
  "status": "OUT_OF_SERVICE",
  "components": {
    "ping": {
      "status": "UP"
    },
    "ssl": {
      "status": "OUT_OF_SERVICE",
      "details": {
        "certificates": [
          {
            "version": "V3",
            "validity": {
              "status": "EXPIRED",
              "message": "Not valid after 2014-10-21T19:48:43Z"
            },
            "validityStarts": "2014-07-23T19:48:43Z",
            "validityEnds": "2014-10-21T19:48:43Z",
            "signatureAlgorithmName": "SHA256withRSA",
            "serialNumber": "7207ee6e",
            "issuer": "CN=localhost,OU=Unknown,O=Unknown,L=Unknown,ST=Unknown,C=Unknown",
            "subject": "CN=localhost,OU=Unknown,O=Unknown,L=Unknown,ST=Unknown,C=Unknown"
          }
        ]
      }
    }
  }
}
/info of a cert that WILL_EXPIRE_SOON (click here to expand)
{
  "ssl": {
    "bundles": [
      {
        "name": "ssldemo",
        "certificateChains": [
          {
            "alias": "spring-boot",
            "certificates": [
              {
                "version": "V3",
                "validityEnds": "2024-06-22T21:32:22Z",
                "validityStarts": "2024-06-21T21:32:22Z",
                "signatureAlgorithmName": "SHA256withRSA",
                "validity": {
                  "status": "WILL_EXPIRE_SOON",
                  "message": "Certificate will expire within threshold (PT168H) at 2024-06-22T21:32:22Z"
                },
                "subject": "CN=localhost,OU=Spring,O=VMware,L=Palo Alto,ST=California,C=US",
                "serialNumber": "64d019d1dd94eee0",
                "issuer": "CN=localhost,OU=Spring,O=VMware,L=Palo Alto,ST=California,C=US"
              }
            ]
          }
        ]
      }
    ]
  }
}
/health of a cert that WILL_EXPIRE_SOON (click here to expand)
{
  "status": "UP",
  "components": {
    "ping": {
      "status": "UP"
    },
    "ssl": {
      "description": "One of the certificates will expire within the defined threshold.",
      "status": "WILL_EXPIRE_SOON",
      "details": {
        "certificates": [
          {
            "version": "V3",
            "validityEnds": "2024-06-22T21:32:22Z",
            "validityStarts": "2024-06-21T21:32:22Z",
            "signatureAlgorithmName": "SHA256withRSA",
            "validity": {
              "status": "WILL_EXPIRE_SOON",
              "message": "Certificate will expire within threshold (PT168H) at 2024-06-22T21:32:22Z"
            },
            "subject": "CN=localhost,OU=Spring,O=VMware,L=Palo Alto,ST=California,C=US",
            "serialNumber": "64d019d1dd94eee0",
            "issuer": "CN=localhost,OU=Spring,O=VMware,L=Palo Alto,ST=California,C=US"
          }
        ]
      }
    }
  }
}

If you want to play with it, start spring-boot-smoke-test-tomcat-ssl, the cert in resources/sample.jks is already EXPIRED, you can generate a VALID one via

keytool -genkeypair -storepass secret -keypass password -keystore sample.jks -storetype JKS -dname "CN=localhost, OU=Spring, O=VMware, L=Palo Alto, ST=California, C=US" -validity 14 -alias spring-boot -keyalg RSA -ext "SAN=DNS:localhost,IP:::1,IP:127.0.0.1"

or one that WILL_EXPIRE_SOON via:

keytool -genkeypair -storepass secret -keypass password -keystore sample.jks -storetype JKS -dname "CN=localhost, OU=Spring, O=VMware, L=Palo Alto, ST=California, C=US" -validity 1 -alias spring-boot -keyalg RSA -ext "SAN=DNS:localhost,IP:::1,IP:127.0.0.1"

@jonatan-ivanov jonatan-ivanov added the status: waiting-for-triage An issue we've not yet triaged label Jun 21, 2024
@jonatan-ivanov jonatan-ivanov mentioned this pull request Jun 21, 2024
15 tasks
@mhalbritter
Copy link
Contributor

mhalbritter commented Jun 27, 2024

Hey @jonatan-ivanov, that's quite a cool feature, thank you! The implementation looks good, too.

This works for all SSL bundles, and not only for the one used by the webserver, right?

@jonatan-ivanov
Copy link
Member Author

Great to hear! The main focus is the webserver but I think this should work for all SSL bundles (not tested yet) since using an expired cert can cause issues in every place they are used.

I can go ahead and work on the TODO items/docs/tests, in the meantime, can I get some feedback on two important items?

  1. Can/should we introduce a new (common) health status: WARNING (returns 200 but indicates that something is not right)? Or should we keep the current custom status in the PR (WILL_EXPIRE_SOON_STATUS)?
  2. Is calling WebServerSslBundle.get multiple times ok? See in the PR, in AbstractConfigurableWebServerFactory, and in NettyRSocketServerFactory or should there be just one instance (i.e.: a @Bean)?

@scottfrederick
Copy link
Contributor

Is calling WebServerSslBundle.get multiple times ok?

I think this is fine. However, I'm not sure we need to use WebServerSslBundle here.

That class adapts the discrete server.ssl.* properties to SslBundle design. If we ever decide to deprecate and remove the discrete server.ssl.* properties and only support bundles, then that class would go away.

We could just focus the InfoContributor on SSL bundles and document that you would need to convert from server.ssl.* properties to bundle properties to get the benefit of the InfoContributor.

@jonatan-ivanov
Copy link
Member Author

That class adapts the discrete server.ssl.* properties to SslBundle design. If we ever decide to deprecate and remove the discrete server.ssl.* properties and only support bundles, then that class would go away.

👍🏼 That's the exact same use-case I'm using WebServerSslBundle in this PR for: to be backwards compatible with the server.ssl.* properties. If only bundles will be supported and WebServerSslBundle will be removed in the future, the small block of code in SslInfo where WebServerSslBundle is used should also be removed since there won't be any properties to be backward compatible with.

We could just focus the InfoContributor on SSL bundles and document that you would need to convert from server.ssl.* properties to bundle properties to get the benefit of the InfoContributor.

That would simplify the changes in the PR a bit but also complicate the life of the users: simply enabling the feature might not do anything for them if they are not using bundles or it would work just half-way for them if they use bundles just not for the webserver. If it is not a big issue to use WebServerSslBundle, I would prefer supporting the server.ssl.* properties unless you have plans to deprecate them (and I guess eventually remove them).

@scottfrederick
Copy link
Contributor

Along with server.ssl.* (and management.server.ssl.*), we have spring.rsocket.server.ssl.*, spring.rabbitmq.ssl.*, and spring.kafka.*.ssl.* properties where users can currently choose to use discrete keystore and truststore properties or use a bundle. The web servers are the only place where we have an adapter like WebServerSslBundle that can be used to easily implement support for the discrete properties in the InfoContributor. We could choose to treat the discrete web server SSL properties specially as you've done by using the adapter (they are almost certainly the most commonly used SSL properties), or only support bundles so everything is treated equally and document that only bundles are supported. Let's see what others think about this.

In either case, I don't think it's necessary to make WebServerSslBundle a bean.

@jonatan-ivanov
Copy link
Member Author

Treating the discrete web server SSL properties specially somewhat makes sense to me since that's what WebServerSslBundle is also doing but having more "non-bundle" properties (I forgot about rabbitmq, kafka, and rsocket) drives me to the other direction: even if it would be nice to have backward-compatible support for the web server properties, it might make more sense to only support bundles and treat everything equally. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants