fix(authn/basic): Enable basic auth in gate #675

german-muzquiz · 2018-11-29T21:33:51Z

This PR is for being able to use plain basic authentication in gate.
Example gate-local.yml:

security:
  basicform:
    enabled: true
  user:
    name: {some username}
    password: {some password}

spinnakerbot · 2018-11-29T21:35:20Z

We prefer that non-test backend code be written in Java or Kotlin, rather than Groovy. The following files have been added and written in Groovy:

gate-basic/src/main/groovy/com/netflix/spinnaker/gate/security/basic/BasicSsoConfig.groovy

See our server-side commit conventions here.

spinnakerbot · 2018-11-29T21:35:21Z

The following commits need their title changed:

469f8ea: Enable basic auth

Please format your commit title into the form:

<type>(<scope>): <subject>, e.g. fix(kubernetes): address NPE in status check

This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here.

ethanfrogers · 2018-11-29T21:50:34Z

cc @ttomsu

ttomsu · 2018-11-30T16:49:14Z

gate-basic/src/main/groovy/com/netflix/spinnaker/gate/security/basic/BasicSsoConfig.groovy

+@EnableWebSecurity
+@SuppportsMultiAuth
+@Order(Ordered.LOWEST_PRECEDENCE)
+class BasicSsoConfig extends WebSecurityConfigurerAdapter {


Rm "Sso" - nothing SSO about HTTP Basic auth.

ttomsu · 2018-11-30T16:51:17Z

gate-basic/src/main/groovy/com/netflix/spinnaker/gate/security/basic/BasicSsoConfig.groovy

+@Configuration
+@SpinnakerAuthConfig
+@EnableWebSecurity
+@SuppportsMultiAuth


I think you'll want to remove this - it's for use with X509, and I think the standalone X509 one should already hook into the Basic config done in AuthConfig

ttomsu · 2018-11-30T16:53:38Z

gate-basic/src/main/groovy/com/netflix/spinnaker/gate/security/basic/BasicSsoConfig.groovy

+
+  @Override
+  protected void configure(HttpSecurity http) throws Exception {
+    http.formLogin()


Hmm. This is more than just HTTP Basic auth - it's actually just simple in memory auth that happens to (maybe) also enable HTTP Basic.

Given that, I think you'll want to rename the module and classes to something like "Simple" or "NonProd". Thoughts on naming @stewchen @dibyom ?

ttomsu · 2018-11-30T16:54:10Z

gate-basic/src/main/groovy/com/netflix/spinnaker/gate/security/basic/BasicSsoConfig.groovy

+  protected void configure(HttpSecurity http) throws Exception {
+    http.formLogin()
+    authConfig.configure(http)
+    additionalAuthProviders?.each {


As mentioned above, I don't think you want to mix this auth mechanism with other ones, so you should probably remove this

ttomsu · 2018-11-30T16:56:37Z

@lwander FYI

german-muzquiz · 2018-11-30T17:55:52Z

@ttomsu I removed multi-auth for this module if it's not needed to coexist with X509. Yeah, basically this is an "in-memory" authentication, so let me know what name would be better suited.

dibyom

I like BasicAuth or maybe "Simple" for the name

dibyom · 2018-11-30T18:09:45Z

gradle.properties

@@ -1,2 +1,2 @@
 org.gradle.parallel=true
-includeProviders=iap,ldap,oauth2,saml,x509
+includeProviders=iap,ldap,oauth2,saml,x509,basic


Very minor nit...arrange in alphabetical order

dibyom · 2018-11-30T18:09:56Z

settings.gradle

@@ -23,7 +23,8 @@ include "gate-core",
  "gate-proxy",
  "gate-saml",
  "gate-web",
-  "gate-x509"
+  "gate-x509",
+  "gate-basic"


Very minor nit...arrange in alphabetical order

german-muzquiz · 2018-12-10T22:50:40Z

@ttomsu all feedback has been implemented unless there's a better name than "BasicAuth" for this functionality. Do you think this PR is ready to merge?

ttomsu · 2018-12-12T20:48:10Z

Could you add a @GateSystemTest like https://github.com/spinnaker/gate/blob/master/gate-web/src/test/groovy/com/netflix/spinnaker/gate/security/ldap/LdapAuthSpec.groovy to prove it's working as you'd expect?

german-muzquiz · 2018-12-12T22:46:23Z

Tests were added and looks that everything is ok.

robzienert

Thanks for the contribution! Some small stuff to address.

robzienert · 2018-12-13T17:38:39Z

gate-basic/src/main/groovy/com/netflix/spinnaker/gate/security/basic/BasicAuthConfig.groovy

@@ -0,0 +1,60 @@
+/*


Please refrain from introducing new Groovy files in the main source set, as the spinnakerbot called out.

robzienert · 2018-12-13T17:39:19Z

gate-basic/src/main/groovy/com/netflix/spinnaker/gate/security/basic/BasicAuthProvider.groovy

+    String password = authentication.getCredentials()?.toString()
+
+    if (securityProperties.user == null) {
+      throw new AuthenticationServiceException("User credentials are not configured for the service")


I'd remove for the service, as it's redundant.

Additionally, this looks like a configuration error that should be caught on startup, rather than asserted every authenticate invocation. I'd move this check into the constructor and force gate to fail on start up.

robzienert · 2018-12-13T17:40:11Z

gate-basic/src/main/groovy/com/netflix/spinnaker/gate/security/basic/BasicAuthConfig.groovy

+  AuthConfig authConfig
+
+  @Autowired
+  BasicAuthProvider authProvider


Also, we're trying to steer people away from autowired properties in favor of autowired constructors.

robzienert · 2018-12-13T17:42:10Z

gate-web/gate-web.gradle

@@ -43,6 +43,7 @@ dependencies {
  compileOnly spinnaker.dependency("lombok")

  testCompile project(":gate-ldap") // TODO: Move system tests to own module
+  testCompile project(":gate-basic")


I think you'll want this to be compile project(":gate-basic"), otherwise the functionality won't be included in the compiled jar.

For that case, all gate auth modules are already included as runtime dependencies, and the list of those is built from gradle.properties: https://github.com/spinnaker/gate/pull/675/files/36ed77203133a36a5a25e83b77e67965138db711#diff-503f218d646c10f484fdc9d6315bf2e3R2
These test dependencies are only there because of some tests written against ldap and basic implementations.

Oh, right on thanks!

robzienert

lgtm. another one-over @ttomsu ?

ttomsu

One more thing: have you tested this out with another auth provider enabled, such as Oauth? I know we use security.basic.enabled to allow the monitoring daemon access to Gate in a kubernetes environment.

german-muzquiz · 2019-01-02T18:47:04Z

@ttomsu I see, I did a test with oauth2 enabled and basic security is taking precedence, because security.basic.enabled is already set to true by some other mechanism.
Given that we have full control over this property name to enable/disable basic auth, I did a test changing it to some other name and oauth2 worked, so instead of security.basic.enabled we could use something like security.basicform.enabled.
What do you think?

german-muzquiz · 2019-01-03T17:33:53Z

@ttomsu @robzienert I went ahead and changed the activation property name for this feature to security.basicform.enabled to not collide with other auth configurations. Also made sure that it works properly with security.basic enabled/disabled.

ethanfrogers · 2019-01-07T19:28:06Z

I'm going to go ahead and merge this since it's been doubly approved.

fix(authn/basic): Enable basic auth

3f29732

german-muzquiz force-pushed the fix-basic-auth branch from 469f8ea to 3f29732 Compare November 29, 2018 21:43

ttomsu requested changes Nov 30, 2018

View reviewed changes

ttomsu requested review from dibyom and stewchen November 30, 2018 16:56

fix(authn/basic): Removed multiauth support from basic authn

36ed772

dibyom reviewed Nov 30, 2018

View reviewed changes

German Muzquiz added 2 commits November 30, 2018 12:28

fix(authn/basic): Arranged module names in alphabetical order

8254966

Merge branch 'master' of github.com:spinnaker/gate into fix-basic-auth

763deaf

German Muzquiz added 2 commits December 12, 2018 12:40

fix(authn/basic): Spinnaker User principal object instead of a string

0d1e01d

Merge branch 'master' of github.com:spinnaker/gate into fix-basic-auth

af2a86f

German Muzquiz added 2 commits December 12, 2018 14:15

fix(authn/basic): Added tests for basic auth

25b5967

Merge branch 'master' of github.com:spinnaker/gate into fix-basic-auth

1266c49

robzienert requested changes Dec 13, 2018

View reviewed changes

fix(authn/basic): Refactor from groovy to java

521b0c5

robzienert approved these changes Dec 14, 2018

View reviewed changes

German Muzquiz added 3 commits December 17, 2018 08:54

Merge branch 'master' of github.com:spinnaker/gate into fix-basic-auth

38f50c4

Merge branch 'master' of github.com:spinnaker/gate into fix-basic-auth

c700cc3

Merge branch 'master' of github.com:spinnaker/gate into fix-basic-auth

92670b9

ttomsu approved these changes Dec 28, 2018

View reviewed changes

Merge branch 'master' of github.com:spinnaker/gate into fix-basic-auth

98d003a

German Muzquiz added 4 commits January 3, 2019 09:09

fix(authn/basic): Renamed property for enabling basic auth

2a3548f

Merge branch 'master' of github.com:spinnaker/gate into fix-basic-auth

6d4ce8c

fix(authn/basic): Fixed and added tests for security.basic combinations

349376a

Merge branch 'master' of github.com:spinnaker/gate into fix-basic-auth

5ac9d5d

Merge branch 'master' into fix-basic-auth

bcc2578

ethanfrogers merged commit 9989cb6 into spinnaker:master Jan 7, 2019

ethanfrogers deleted the fix-basic-auth branch January 7, 2019 19:28

spinnakerbot added the target-release/1.12 label Jan 7, 2019

german-muzquiz mentioned this pull request Feb 26, 2019

Kb article enabling simple auth armory/knowledge-base#106

Closed

german-muzquiz mentioned this pull request Aug 28, 2019

REQUEST: New Reviewer status for german-muzquiz spinnaker/governance#26

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(authn/basic): Enable basic auth in gate #675

fix(authn/basic): Enable basic auth in gate #675

german-muzquiz commented Nov 29, 2018 •

edited

spinnakerbot commented Nov 29, 2018

spinnakerbot commented Nov 29, 2018

ethanfrogers commented Nov 29, 2018

ttomsu Nov 30, 2018

ttomsu Nov 30, 2018

ttomsu Nov 30, 2018

ttomsu Nov 30, 2018

ttomsu commented Nov 30, 2018

german-muzquiz commented Nov 30, 2018

dibyom left a comment •

edited

dibyom Nov 30, 2018

dibyom Nov 30, 2018

german-muzquiz commented Dec 10, 2018

ttomsu commented Dec 12, 2018

german-muzquiz commented Dec 12, 2018

robzienert left a comment

robzienert Dec 13, 2018

robzienert Dec 13, 2018

robzienert Dec 13, 2018

robzienert Dec 13, 2018

robzienert Dec 13, 2018

german-muzquiz Dec 14, 2018

robzienert Dec 14, 2018

robzienert left a comment

ttomsu left a comment

german-muzquiz commented Jan 2, 2019

german-muzquiz commented Jan 3, 2019

ethanfrogers commented Jan 7, 2019

fix(authn/basic): Enable basic auth in gate #675

fix(authn/basic): Enable basic auth in gate #675

Conversation

german-muzquiz commented Nov 29, 2018 • edited

spinnakerbot commented Nov 29, 2018

spinnakerbot commented Nov 29, 2018

ethanfrogers commented Nov 29, 2018

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

ttomsu commented Nov 30, 2018

german-muzquiz commented Nov 30, 2018

dibyom left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

german-muzquiz commented Dec 10, 2018

ttomsu commented Dec 12, 2018

german-muzquiz commented Dec 12, 2018

robzienert 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robzienert left a comment

Choose a reason for hiding this comment

ttomsu left a comment

Choose a reason for hiding this comment

german-muzquiz commented Jan 2, 2019

german-muzquiz commented Jan 3, 2019

ethanfrogers commented Jan 7, 2019

german-muzquiz commented Nov 29, 2018 •

edited

dibyom left a comment •

edited