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

Allow to disable SSL client authentication on the management port #14985

Closed
wants to merge 1 commit into from
Closed

Conversation

alonbl
Copy link
Contributor

@alonbl alonbl commented Oct 28, 2018

When server and management are at different ports, and when server requires
TLS client authentication, then there is no simple method to disable TLS
client authentication for management port.

Adding ssl.client-auth=none enables this.

Example:

server.port=8080
server.ssl.enabled=true
server.ssl.client-auth=need
management.server.port=8081
management.server.ssl.enabled=true
management.server.ssl.client-auth=none

Signed-off-by: Alon Bar-Lev alon.barlev@gmail.com

When server and management are at different ports, and when server requires
TLS client authentication, then there is no simple method to disable TLS
client authentication for management port.

Adding ssl.client-auth=none enables this.

Example:

    server.port=8080
    server.ssl.enabled=true
    server.ssl.client-auth=need
    management.server.port=8081
    management.server.ssl.enabled=true
    management.server.ssl.client-auth=none

Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
@pivotal-issuemaster
Copy link

@alonbl Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 28, 2018
@pivotal-issuemaster
Copy link

@alonbl Thank you for signing the Contributor License Agreement!

@snicoll
Copy link
Member

snicoll commented Nov 1, 2018

@alonbl it's not very clear what this PR does. If you specify a SSL for server.ssl.client-auth it won't have an impact on the management config AFAIK. Not having a value already translates to SslClientAuthMode.NOT_REQUESTED.

It would have helped to add a test that exercises the intended change. Am I missing something here?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Nov 1, 2018
@alonbl
Copy link
Contributor Author

alonbl commented Nov 3, 2018

@snicoll: thanks!

Here is the problem... maybe I am doing wrong solution as I am not deep into spring-boot but more security. And I see now that this is insufficient.

I have the following configuration:

server:
  ssl:
    enabled: true
    key-store: file:///tmp/server1.p12
    key-store-password: changeit
management:
  server:
    port: 8081

When I try to access management port, I get:

$ curl -H -k https://localhost:8081/actuator/info
{}

Now I set:

server:
  ssl:
    enabled: true
    client-auth: need
    key-store: file:///tmp/server1.p12
    key-store-password: changeit
management:
  server:
    port: 8081

And try again:

$ curl -H -k https://localhost:8081/actuator/info
curl: (35) error:14094412:SSL routines:ssl3_read_bytes:sslv3 alert bad certificate

This means that the SSL setting of server is copied to the management.
Am I right so far?
If so, then I try to disable the setting, using:

server:
  ssl:
    enabled: true
    client-auth: need
    key-store: file:///tmp/server1.p12
    key-store-password: changeit
management:
  server:
    port: 8081
    ssl:
      client-auth:

Same result, so having an empty is not the same as reverting to null.

I expect to be able to write something like:

server:
  ssl:
    enabled: true
    client-auth: need
    key-store: file:///tmp/server1.p12
    key-store-password: changeit
management:
  server:
    port: 8081
    ssl:
      client-auth: none

However, I see that every variable at management.server.ssl forces to specify all settings, which is not what I expect, as I do like the default behaviour of having a baseline intact.

Based on that, I thought I will write:

server:
  ssl:
    enabled: true
    client-auth: need
    key-store: file:///tmp/server1.p12
    key-store-password: changeit
management:
  server:
    port: 8081
    ssl:
      ciphers: ${server.ssl.ciphers:}
      #client-auth: ${server.ssl.client-auth:}
      enabled: ${server.ssl.enabled:}
      enabled-protocols: ${server.ssl.enabled-protocols:}
      key-alias: ${server.ssl.key-alias:}
      key-password: ${server.ssl.key-password:}
      key-store: ${server.ssl.key-store:}
      key-store-password: ${server.ssl.key-store-password:}
      key-store-provider: ${server.ssl.key-store-provider:}
      key-store-type: ${server.ssl.key-store-type:}
      protocol: ${server.ssl.protocol:}
      trust-store: ${server.ssl.trust-store:}
      trust-store-password: ${server.ssl.trust-store-password:}
      trust-store-provider: ${server.ssl.trust-store-provider:}
      trust-store-type: ${server.ssl.trust-store-type:}

However, the full environment of server.ssl is not available for these variables that are not explicitly set, so substitution fails.

Summary: be able to inherit all settings of server.ssl into management.ssl and allow override individual keys, mainly the enabled and client-auth, maybe these should not be in the server.ssl and move to server.{ssl-enabled,ssl-client-auth}, but this will break existing applications.

Can you think of another solution in which I can configure client-auth: need for server and not for management without copying existing properties and/or be immune when someone adds additional properties to server, as I probably want these to be applied to management as well.

Thanks!

@snicoll snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Nov 5, 2018
@bclozel
Copy link
Member

bclozel commented Nov 5, 2018

In that space, the current behavior is (see ManagementWebServerFactoryCustomizer):

  • if you don't specify anything for management.server.ssl.*, then the server customizer for server.ssl.* is applied and the management server replicates the main server SSL configuration.
  • if you specified anything for management.server.ssl.*, then this configuration overrides the server.ssl.* one.

So I believe this PR can actually be useful, since apparently there is no way to only disable that part of the SSL configuration for the management server.

@bclozel bclozel added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 5, 2018
@bclozel bclozel added this to the 2.0.x milestone Nov 5, 2018
@snicoll snicoll changed the title web: ssl: support ssl.client-auth=none Allow to disable SSL on the management port Nov 5, 2018
@snicoll snicoll modified the milestones: 2.0.x, 2.1.x Nov 28, 2018
@snicoll snicoll self-assigned this Nov 28, 2018
@snicoll snicoll changed the title Allow to disable SSL on the management port Allow to disable SSL client authentication on the management port Nov 28, 2018
@snicoll snicoll modified the milestones: 2.1.x, 2.1.1 Nov 28, 2018
snicoll pushed a commit that referenced this pull request Nov 28, 2018
When server and management are at different ports, and when server
requires TLS client authentication, then there is no simple method to
disable TLS client authentication for management port.

This commit adds an additional "none" option to ssl.client-auth.

Example:

    server.port=8080
    server.ssl.enabled=true
    server.ssl.client-auth=need
    management.server.port=8081
    management.server.ssl.enabled=true
    management.server.ssl.client-auth=none

See gh-14985
@snicoll snicoll closed this in 33000b6 Nov 28, 2018
snicoll added a commit that referenced this pull request Nov 28, 2018
* pr/14985:
  Polish contribution
  Allow to disable SSL client authentication on the management port
@snicoll
Copy link
Member

snicoll commented Nov 28, 2018

@alonbl thank you very much for making your first contribution to Spring Boot. I've merged this to master with a polish commit

@alonbl
Copy link
Contributor Author

alonbl commented Nov 28, 2018

@snicoll : Thanks!

Now we should think of how to allow setting individual setting in management instead of all-or-nothing copy from the server. Should I open a PR for this discussion?

@bclozel
Copy link
Member

bclozel commented Nov 28, 2018

@alonbl An issue probably, as I'm not sure how this could be achieved without breaking backwards compatibility or making things more complex than necessary.

Are you suggesting that management.server.ssl.* should inherit server.ssl.* by default and keys should only override what's inherited?

@alonbl
Copy link
Contributor Author

alonbl commented Nov 28, 2018

@bclozel: I suggest to have:

server.ssl-client-auth as an override of server.ssl.client-auth
management.server.ssl-client-auth as an override of management.server.ssl.client-auth

Maybe bad naming, but the principal is to move this setting out of the ssl, while maintaining backward compatibility, leaving ssl to only contain the settings for the crypto of the ssl.

Any other idea?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants