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

Consider making the default user's username and password configurable #10963

Closed
wilkinsona opened this Issue Nov 9, 2017 · 30 comments

Comments

Projects
None yet
8 participants
@wilkinsona
Member

wilkinsona commented Nov 9, 2017

Please see the discussion that starts here for details.

AuthenticationManagerConfiguration could consume some properties for configuring the default user's username and password. I'm not sure how useful it would be as it'll quickly back off when the user sets up their own security. Perhaps it's still of sufficient value for demos and the out-of-the-box experience as users are finding their feet?

@mbhave

This comment has been minimized.

Show comment
Hide comment
@mbhave

mbhave Nov 9, 2017

Contributor

I agree that it's only useful for demos/OOTB since real-world use-cases wouldn't use a single user. I don't particularly see any harm in bringing the property back but we should be careful about naming it since security.basic.user is no longer applicable.

Contributor

mbhave commented Nov 9, 2017

I agree that it's only useful for demos/OOTB since real-world use-cases wouldn't use a single user. I don't particularly see any harm in bringing the property back but we should be careful about naming it since security.basic.user is no longer applicable.

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Nov 9, 2017

Member

Perhaps we could allow more than one user to be created? Boot would just create a Map<String,String> and then delegate to a Spring Security's UserDetailsMapFactoryBean that creates a Collection<UserDetails> from that Map<String,String>.

That would mean users could provide something like this:

spring:
  security:
    users:
        user: {noop}password,ROLE_USER
        admin: {noop}password,ROLE_USER,ROLE_ADMIN

This has the advantage of being able to provide multiple users and configure any User property. The disadvantage is that it might be more difficult to remember the syntax and the username and roles must be provided.

Member

rwinch commented Nov 9, 2017

Perhaps we could allow more than one user to be created? Boot would just create a Map<String,String> and then delegate to a Spring Security's UserDetailsMapFactoryBean that creates a Collection<UserDetails> from that Map<String,String>.

That would mean users could provide something like this:

spring:
  security:
    users:
        user: {noop}password,ROLE_USER
        admin: {noop}password,ROLE_USER,ROLE_ADMIN

This has the advantage of being able to provide multiple users and configure any User property. The disadvantage is that it might be more difficult to remember the syntax and the username and roles must be provided.

@mbhave

This comment has been minimized.

Show comment
Hide comment
@mbhave

mbhave Nov 9, 2017

Contributor

There was a similar discussion here

Contributor

mbhave commented Nov 9, 2017

There was a similar discussion here

@snicoll

This comment has been minimized.

Show comment
Hide comment
@snicoll

snicoll Nov 13, 2017

Member

Thanks @mbhave, that discussion summarizes our concerns with map-based binding and magic strings breaking IDE support.

I know @michael-simons has built a starter to bring back the single user configuration. Perhaps he can chime in and share his use cases?

Member

snicoll commented Nov 13, 2017

Thanks @mbhave, that discussion summarizes our concerns with map-based binding and magic strings breaking IDE support.

I know @michael-simons has built a starter to bring back the single user configuration. Perhaps he can chime in and share his use cases?

@michael-simons

This comment has been minimized.

Show comment
Hide comment
@michael-simons

michael-simons Nov 13, 2017

Contributor

Hi.
My starter is here, still on M5.

My use case is actually the single user, in several applications. For those it's enough to configure username, password and roles.

@dasniko had another production use case he described to me… I think it was passing the user properties from an external service during startup or something like that.

Contributor

michael-simons commented Nov 13, 2017

Hi.
My starter is here, still on M5.

My use case is actually the single user, in several applications. For those it's enough to configure username, password and roles.

@dasniko had another production use case he described to me… I think it was passing the user properties from an external service during startup or something like that.

@philwebb

This comment has been minimized.

Show comment
Hide comment
@philwebb

philwebb Nov 14, 2017

Member

I don't mind properties to allow a single user to be configured quickly. It's going to help with migration, give us a nicer learning experience and it seems like it could be useful for demos. I'm tempted to say we should steer clear of the full Map based approach since it complicates things quite a bit.

Member

philwebb commented Nov 14, 2017

I don't mind properties to allow a single user to be configured quickly. It's going to help with migration, give us a nicer learning experience and it seems like it could be useful for demos. I'm tempted to say we should steer clear of the full Map based approach since it complicates things quite a bit.

@philwebb

This comment has been minimized.

Show comment
Hide comment
@philwebb

philwebb Nov 15, 2017

Member

We'll add properties back for single user support.

Member

philwebb commented Nov 15, 2017

We'll add properties back for single user support.

@snicoll snicoll added this to the 2.0.0.RC1 milestone Nov 15, 2017

@wilkinsona

This comment has been minimized.

Show comment
Hide comment
@wilkinsona

wilkinsona Nov 23, 2017

Member

When we add this back, we need to update this section of the migration guide.

Member

wilkinsona commented Nov 23, 2017

When we add this back, we need to update this section of the migration guide.

@mbhave mbhave self-assigned this Dec 11, 2017

@mbhave mbhave closed this in 47ed096 Dec 11, 2017

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Dec 11, 2017

Member

@mbhave Thanks for the updates. Right now the changes mean that the user cannot provide an encoded password which decreases the security. From a security perspective, only the generated password would be encoded and the user provided one would already be encoded (so that if the password is compromised it is only the encoded password).

Member

rwinch commented Dec 11, 2017

@mbhave Thanks for the updates. Right now the changes mean that the user cannot provide an encoded password which decreases the security. From a security perspective, only the generated password would be encoded and the user provided one would already be encoded (so that if the password is compromised it is only the encoded password).

@mbhave

This comment has been minimized.

Show comment
Hide comment
@mbhave

mbhave Dec 11, 2017

Contributor

Reopening this issue to discuss password encoding. It makes sense that the password specified through properties files or environment variables is encoded. However, the purpose of adding a single user was for ease of configuring the username/password for demos. Encoding the password for those use-cases might be a little tedious. Those who don't want password encoding can add a bean of NoOpPasswordEncoder, but that almost seems as much effort as configuring your own UserDetailsService. The other option is to specify {noop}mypassword.

Contributor

mbhave commented Dec 11, 2017

Reopening this issue to discuss password encoding. It makes sense that the password specified through properties files or environment variables is encoded. However, the purpose of adding a single user was for ease of configuring the username/password for demos. Encoding the password for those use-cases might be a little tedious. Those who don't want password encoding can add a bean of NoOpPasswordEncoder, but that almost seems as much effort as configuring your own UserDetailsService. The other option is to specify {noop}mypassword.

@mbhave mbhave reopened this Dec 11, 2017

@snicoll

This comment has been minimized.

Show comment
Hide comment
@snicoll

snicoll Dec 12, 2017

Member

+1 to what @mbhave said.

Member

snicoll commented Dec 12, 2017

+1 to what @mbhave said.

snicoll added a commit that referenced this issue Dec 12, 2017

@philwebb

This comment has been minimized.

Show comment
Hide comment
@philwebb

philwebb Dec 12, 2017

Member

I very much dislike the {noop}password syntax for properties files, I think it's a little unintuitive. If we need to support encoded properties could we consider an additional property? That would be a little more discoverable and we'd be able to provide meta-data hints:

spring.security.user.password-encoding=bcrypt # default is noop
spring.security.user.password=$2a$10$dXJ3SW6G7P50lGmMkkmwe.20cQQubK3.HZWzG3YB1tlRy.fqvM/BG
Member

philwebb commented Dec 12, 2017

I very much dislike the {noop}password syntax for properties files, I think it's a little unintuitive. If we need to support encoded properties could we consider an additional property? That would be a little more discoverable and we'd be able to provide meta-data hints:

spring.security.user.password-encoding=bcrypt # default is noop
spring.security.user.password=$2a$10$dXJ3SW6G7P50lGmMkkmwe.20cQQubK3.HZWzG3YB1tlRy.fqvM/BG
@mbhave

This comment has been minimized.

Show comment
Hide comment
@mbhave

mbhave Dec 12, 2017

Contributor

I'm not sure about adding more properties for the single user use-case. From what I can tell, it seems useful only for demos and I think we should document it as such. If someone wants to use a single user in production and encode the password, they should consider adding their own UserDetailsService with an encoded password.

Contributor

mbhave commented Dec 12, 2017

I'm not sure about adding more properties for the single user use-case. From what I can tell, it seems useful only for demos and I think we should document it as such. If someone wants to use a single user in production and encode the password, they should consider adding their own UserDetailsService with an encoded password.

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Dec 12, 2017

Member

spring.security.user.password-encoding=bcrypt # default is noop

@philwebb We really should not have an insecure default of noop. We want to make it somewhat painful to do the wrong thing (i.e. noop) because it is not considered best practice.

Another thing we can do to make encoding the passwords easier would be to add a tool in the Spring CLI tooling to perform the encoding for the user. If Boot does not provide such tooling, Spring Security will be doing so eventually.

Member

rwinch commented Dec 12, 2017

spring.security.user.password-encoding=bcrypt # default is noop

@philwebb We really should not have an insecure default of noop. We want to make it somewhat painful to do the wrong thing (i.e. noop) because it is not considered best practice.

Another thing we can do to make encoding the passwords easier would be to add a tool in the Spring CLI tooling to perform the encoding for the user. If Boot does not provide such tooling, Spring Security will be doing so eventually.

@michael-simons

This comment has been minimized.

Show comment
Hide comment
@michael-simons

michael-simons Dec 12, 2017

Contributor

Current state is plain text passwords for the single user in config files. Seems to have been fine so far. I wouldn’t change that / open a box.

Contributor

michael-simons commented Dec 12, 2017

Current state is plain text passwords for the single user in config files. Seems to have been fine so far. I wouldn’t change that / open a box.

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Dec 12, 2017

Member

@michael-simons The old behavior was an insecure default that has been fixed in Spring Security 5.
If users really want plain text passwords they can still do so by exposing a single Bean. However, we should not make insecure defaults as it leads users into thinking that using plain text passwords is a good idea.

Member

rwinch commented Dec 12, 2017

@michael-simons The old behavior was an insecure default that has been fixed in Spring Security 5.
If users really want plain text passwords they can still do so by exposing a single Bean. However, we should not make insecure defaults as it leads users into thinking that using plain text passwords is a good idea.

@michael-simons

This comment has been minimized.

Show comment
Hide comment
@michael-simons

michael-simons Dec 12, 2017

Contributor

So the reasoning is that the insecure configuration has not been Spring Boots result but Spring Securitys. In 5 that changed so in Boot that should adapt, too? Understandable.

Contributor

michael-simons commented Dec 12, 2017

So the reasoning is that the insecure configuration has not been Spring Boots result but Spring Securitys. In 5 that changed so in Boot that should adapt, too? Understandable.

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Dec 12, 2017

Member

So the reasoning is that the insecure configuration has not been Spring Boots result but Spring Securitys. In 5 that changed so in Boot that should adapt, too? Understandable.

@michael-simons Correct

Member

rwinch commented Dec 12, 2017

So the reasoning is that the insecure configuration has not been Spring Boots result but Spring Securitys. In 5 that changed so in Boot that should adapt, too? Understandable.

@michael-simons Correct

@philwebb

This comment has been minimized.

Show comment
Hide comment
@philwebb

philwebb Dec 12, 2017

Member

We really should not have an insecure default of noop. We want to make it somewhat painful to do the wrong thing (i.e. noop) because it is not considered best practice.

Defining a user/password in a properties file isn't really best practice either. I assume the usual reason that you don't want to use {noop} is because a compromised database would contain cleartext passwords? If your .properties file gets compromised a lot of information is potentially leaked (DB creds etc).

I feel like we're moving away from the point of this feature. The original goal (as I understood it) was to allow demos to quickly configure a single user. With that in mind, we should probably not make it at all painful to configure the password.

If a user sets:

spring.security.user.password=mysecret

It's not great if they get an error because the values isn't supported by bcrypt.

I think we need to either add this feature and make it clear that we don't recommend it for production, or just not bother and insist that every developer configures things on their own.

Member

philwebb commented Dec 12, 2017

We really should not have an insecure default of noop. We want to make it somewhat painful to do the wrong thing (i.e. noop) because it is not considered best practice.

Defining a user/password in a properties file isn't really best practice either. I assume the usual reason that you don't want to use {noop} is because a compromised database would contain cleartext passwords? If your .properties file gets compromised a lot of information is potentially leaked (DB creds etc).

I feel like we're moving away from the point of this feature. The original goal (as I understood it) was to allow demos to quickly configure a single user. With that in mind, we should probably not make it at all painful to configure the password.

If a user sets:

spring.security.user.password=mysecret

It's not great if they get an error because the values isn't supported by bcrypt.

I think we need to either add this feature and make it clear that we don't recommend it for production, or just not bother and insist that every developer configures things on their own.

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Dec 13, 2017

Member

FWIW....I'm open to it being resolved in a way that ensures we have secure defaults. So if that means using hashed passwords by default or not supporting this feature, it gets my +1.

Member

rwinch commented Dec 13, 2017

FWIW....I'm open to it being resolved in a way that ensures we have secure defaults. So if that means using hashed passwords by default or not supporting this feature, it gets my +1.

@philwebb

This comment has been minimized.

Show comment
Hide comment
@philwebb

philwebb Dec 13, 2017

Member

Perhaps @dsyer wants to comment on this one since he was involved in the initial discussion. My current stance is that since we're currently secure by default, abd the spring.security.user.password is just a convenience for demos, to should keep it as is and add a "not for production" warning.

Member

philwebb commented Dec 13, 2017

Perhaps @dsyer wants to comment on this one since he was involved in the initial discussion. My current stance is that since we're currently secure by default, abd the spring.security.user.password is just a convenience for demos, to should keep it as is and add a "not for production" warning.

@dsyer

This comment has been minimized.

Show comment
Hide comment
@dsyer

dsyer Dec 13, 2017

Member

I think {algorithm}password is fine as a format for properties - the main thing is that I can do it in one line in a config file. I also echo Michael’s comment that it’s not just for demos (there’s a large enough class of simple apps that only need one user, even in production).

Member

dsyer commented Dec 13, 2017

I think {algorithm}password is fine as a format for properties - the main thing is that I can do it in one line in a config file. I also echo Michael’s comment that it’s not just for demos (there’s a large enough class of simple apps that only need one user, even in production).

@dsyer

This comment has been minimized.

Show comment
Hide comment
@dsyer

dsyer Dec 13, 2017

Member

Corollary to that last point: we should document how to do the production grade thing (with e.g. {bcrypt}...) and make sure it works OOTB.

Member

dsyer commented Dec 13, 2017

Corollary to that last point: we should document how to do the production grade thing (with e.g. {bcrypt}...) and make sure it works OOTB.

@vpavic

This comment has been minimized.

Show comment
Hide comment
@vpavic

vpavic Dec 14, 2017

Member

IMO Spring (and by that I mean Spring in whole, not just individual projects like Security or Boot) should steer users towards what's considered a best practice, especially in the domain of security. Having users configure a cleartext password in application properties isn't quite a best practice.

Regarding the approach to document this as a demo only thing, how much would that actually help in practice? I mean, Boot's pretty good at documenting release notes and uses GitHub issue template that clearly states issues are not used for general questions, and yet how many of the opened issues are invalid due to people not bothering to read the release notes or issue template?

The point is, once the feature is out there, people are going to use it the way they see it fit, not only the way it's documented. I pretty much like the approach Boot 2.0 has taken to Spring Security configuration, and this feels like going back to the old way a bit.

Member

vpavic commented Dec 14, 2017

IMO Spring (and by that I mean Spring in whole, not just individual projects like Security or Boot) should steer users towards what's considered a best practice, especially in the domain of security. Having users configure a cleartext password in application properties isn't quite a best practice.

Regarding the approach to document this as a demo only thing, how much would that actually help in practice? I mean, Boot's pretty good at documenting release notes and uses GitHub issue template that clearly states issues are not used for general questions, and yet how many of the opened issues are invalid due to people not bothering to read the release notes or issue template?

The point is, once the feature is out there, people are going to use it the way they see it fit, not only the way it's documented. I pretty much like the approach Boot 2.0 has taken to Spring Security configuration, and this feels like going back to the old way a bit.

@mbhave

This comment has been minimized.

Show comment
Hide comment
@mbhave

mbhave Dec 15, 2017

Contributor

IMO Spring (and by that I mean Spring in whole, not just individual projects like Security or Boot) should steer users towards what's considered a best practice, especially in the domain of security.

Agreed.

The issue was originally intended to do provide an easy way (preferably a one-liner) to specify a single username/password via properties. If we make the encoded password the default, I think most users using these properties will end up specifying {noop} but maybe that's not such a bad trade-off.

Contributor

mbhave commented Dec 15, 2017

IMO Spring (and by that I mean Spring in whole, not just individual projects like Security or Boot) should steer users towards what's considered a best practice, especially in the domain of security.

Agreed.

The issue was originally intended to do provide an easy way (preferably a one-liner) to specify a single username/password via properties. If we make the encoded password the default, I think most users using these properties will end up specifying {noop} but maybe that's not such a bad trade-off.

@philwebb

This comment has been minimized.

Show comment
Hide comment
@philwebb

philwebb Dec 15, 2017

Member

Thanks everyone for the input. For better or worse, I'm going to take the decision that we will support the {algorithm}password format but password alone should be {noop}. We'll document that this isn't best practice and you probably want your own UserDetailsService.

Reasons:

  • Properties files can only configure a single user. Most real apps will need a UserDetailsService anyway.
  • Other passwords in the properties (DB connections etc) all use clear-text. I don't want to confuse users by needing them to add {noop} for just this one.
  • On the off-chance that a real production app only needs a single user and has decided the properties file is the way to go, they can still use the {algorithm}password format.
Member

philwebb commented Dec 15, 2017

Thanks everyone for the input. For better or worse, I'm going to take the decision that we will support the {algorithm}password format but password alone should be {noop}. We'll document that this isn't best practice and you probably want your own UserDetailsService.

Reasons:

  • Properties files can only configure a single user. Most real apps will need a UserDetailsService anyway.
  • Other passwords in the properties (DB connections etc) all use clear-text. I don't want to confuse users by needing them to add {noop} for just this one.
  • On the off-chance that a real production app only needs a single user and has decided the properties file is the way to go, they can still use the {algorithm}password format.

@spring-projects spring-projects locked and limited conversation to collaborators Dec 15, 2017

@spring-projects spring-projects unlocked this conversation Dec 15, 2017

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Dec 15, 2017

Member

I'm going to take the decision that we will support the {algorithm}password format but password alone should be {noop}

I'm not sure I understand. Can you go into more detail about what you plan to do and how it would work?

Member

rwinch commented Dec 15, 2017

I'm going to take the decision that we will support the {algorithm}password format but password alone should be {noop}

I'm not sure I understand. Can you go into more detail about what you plan to do and how it would work?

@philwebb

This comment has been minimized.

Show comment
Hide comment
@philwebb

philwebb Dec 15, 2017

Member

Sorry if it wasn't clear:

Users will be able to specify the password like this out of the box:

spring.security.user.password=mysecret

They'll also be able to do this if they want a single property file based password that's more secure:

spring.security.user.password={bcrypt}$2a$10$dXJ3SW6G7P50lGmMkkmwe.20cQQubK3.HZWzG3YB1tlRy.fqvM/BG

We'll document this is a convenience and what they probably want to do is add their own Java configuration.

Member

philwebb commented Dec 15, 2017

Sorry if it wasn't clear:

Users will be able to specify the password like this out of the box:

spring.security.user.password=mysecret

They'll also be able to do this if they want a single property file based password that's more secure:

spring.security.user.password={bcrypt}$2a$10$dXJ3SW6G7P50lGmMkkmwe.20cQQubK3.HZWzG3YB1tlRy.fqvM/BG

We'll document this is a convenience and what they probably want to do is add their own Java configuration.

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Dec 15, 2017

Member

Thanks for the clarification on what users can do. Do you have plans for how this would be implemented? I wanted to point out that defaulting a password prefix of {noop} would break things if a user provides a custom PasswordEncoder bean. Perhaps defaulting the prefix would only occur if no PasswordEncoder bean is provided?

Member

rwinch commented Dec 15, 2017

Thanks for the clarification on what users can do. Do you have plans for how this would be implemented? I wanted to point out that defaulting a password prefix of {noop} would break things if a user provides a custom PasswordEncoder bean. Perhaps defaulting the prefix would only occur if no PasswordEncoder bean is provided?

@philwebb

This comment has been minimized.

Show comment
Hide comment
@philwebb

philwebb Dec 15, 2017

Member

@rwinch I'm not sure yet, but prefixing {noop} if there is no user defined PasswordEncoder bean seems like the best option.

Member

philwebb commented Dec 15, 2017

@rwinch I'm not sure yet, but prefixing {noop} if there is no user defined PasswordEncoder bean seems like the best option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment