Merge remote-tracking branch 'origin/master' into packages/packagejson

* origin/master: Replaced ubuntu:latest with ubuntu:18.04; specific image version to make grafana build images consistent (#18224) Build: correct verify script (#18236) remote_cache: Fix redis connstr parsing (#18204) Auth: do not expose disabled user disabled status (#18229) Build: Introduce shellcheck (#18081) Docs: Update documentation with new SAML features (#18163)
grafana · Jul 23, 2019 · dc02764 · dc02764
2 parents f3388db + 711f62c
commit dc02764
Show file tree

Hide file tree

Showing 38 changed files with 252 additions and 180 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -127,6 +127,15 @@ jobs:
           name: Lint Go
           command: 'make lint-go'
 
+  shellcheck:
+    machine: true
+    working_directory: ~/go/src/github.com/grafana/grafana
+    steps:
+      - checkout
+      - run:
+          name: ShellCheck
+          command: 'make shellcheck'
+
   test-frontend:
     docker:
       - image: circleci/node:10
@@ -650,6 +659,8 @@ workflows:
           filters: *filter-only-master
       - lint-go:
           filters: *filter-only-master
+      - shellcheck:
+          filters: *filter-only-master
       - test-frontend:
           filters: *filter-only-master
       - test-backend:
@@ -665,6 +676,7 @@ workflows:
             - test-frontend
             - codespell
             - lint-go
+            - shellcheck
             - mysql-integration-test
             - postgres-integration-test
             - build-oss-msi
@@ -677,6 +689,7 @@ workflows:
             - test-frontend
             - codespell
             - lint-go
+            - shellcheck
             - mysql-integration-test
             - postgres-integration-test
           filters: *filter-only-master
@@ -687,6 +700,7 @@ workflows:
             - test-frontend
             - codespell
             - lint-go
+            - shellcheck
             - mysql-integration-test
             - postgres-integration-test
             - build-all-enterprise
@@ -698,6 +712,7 @@ workflows:
             - test-frontend
             - codespell
             - lint-go
+            - shellcheck
             - mysql-integration-test
             - postgres-integration-test
           filters: *filter-only-master
@@ -724,6 +739,8 @@ workflows:
           filters: *filter-only-release
       - lint-go:
           filters: *filter-only-release
+      - shellcheck:
+          filters: *filter-only-release
       - test-frontend:
           filters: *filter-only-release
       - test-backend:
@@ -739,6 +756,7 @@ workflows:
             - test-frontend
             - codespell
             - lint-go
+            - shellcheck
             - mysql-integration-test
             - postgres-integration-test
             - build-oss-msi
@@ -751,6 +769,7 @@ workflows:
             - test-frontend
             - codespell
             - lint-go
+            - shellcheck
             - mysql-integration-test
             - postgres-integration-test
           filters: *filter-only-release
@@ -762,6 +781,7 @@ workflows:
             - test-frontend
             - codespell
             - lint-go
+            - shellcheck
             - mysql-integration-test
             - postgres-integration-test
           filters: *filter-only-release
@@ -772,6 +792,7 @@ workflows:
             - test-frontend
             - codespell
             - lint-go
+            - shellcheck
             - mysql-integration-test
             - postgres-integration-test
           filters: *filter-only-release
@@ -791,6 +812,10 @@ workflows:
             filters: *filter-not-release-or-master
         - lint-go:
             filters: *filter-not-release-or-master
+        - lint-go:
+            filters: *filter-not-release-or-master
+        - shellcheck:
+            filters: *filter-not-release-or-master
         - test-frontend:
             filters: *filter-not-release-or-master
         - test-backend:
@@ -808,6 +833,7 @@ workflows:
               - test-frontend
               - codespell
               - lint-go
+              - shellcheck
               - mysql-integration-test
               - postgres-integration-test
               - cache-server-test
@@ -819,6 +845,7 @@ workflows:
               - test-frontend
               - codespell
               - lint-go
+              - shellcheck
               - mysql-integration-test
               - postgres-integration-test
               - cache-server-test

diff --git a/Dockerfile b/Dockerfile
@@ -33,7 +33,7 @@ ENV NODE_ENV production
 RUN ./node_modules/.bin/grunt build
 
 # Final container
-FROM ubuntu:latest
+FROM ubuntu:18.04
 
 LABEL maintainer="Grafana team <hello@grafana.com>"
 

diff --git a/Makefile b/Makefile
@@ -4,6 +4,7 @@
 
 GO = GO111MODULE=on go
 GO_FILES ?= ./pkg/...
+SH_FILES ?= $(shell find ./scripts -name *.sh)
 
 all: deps build
 
@@ -111,6 +112,11 @@ go-vet:
 
 lint-go: go-vet golangci-lint revive revive-alerting gosec
 
+# with disabled SC1071 we are ignored some TCL,Expect `/usr/bin/env expect` scripts
+shellcheck: $(SH_FILES)
+	@docker run --rm -v "$$PWD:/mnt" koalaman/shellcheck:stable \
+	$(SH_FILES) -e SC1071
+
 run: scripts/go/bin/bra
 	@scripts/go/bin/bra run
 

diff --git a/conf/defaults.ini b/conf/defaults.ini
@@ -409,6 +409,15 @@ max_issue_delay = 90s
 # Duration, for how long the SP's metadata should be valid. Defaults to 48 hours
 metadata_valid_duration = 48h
 
+# Friendly name or name of the attribute within the SAML assertion to use as the user's name
+assertion_attribute_name = displayName
+
+# Friendly name or name of the attribute within the SAML assertion to use as the user's login handle
+assertion_attribute_login = mail
+
+# Friendly name or name of the attribute within the SAML assertion to use as the user's email
+assertion_attribute_email = mail
+
 #################################### Basic Auth ##########################
 [auth.basic]
 enabled = true

diff --git a/conf/sample.ini b/conf/sample.ini
@@ -365,6 +365,15 @@
 # Duration, for how long the SP's metadata should be valid. Defaults to 48 hours.
 ;metadata_valid_duration = 48h
 
+# Friendly name or name of the attribute within the SAML assertion to use as the user's name
+;assertion_attribute_name = displayName
+
+# Friendly name or name of the attribute within the SAML assertion to use as the user's login handle
+;assertion_attribute_login = mail
+
+# Friendly name or name of the attribute within the SAML assertion to use as the user's email
+;assertion_attribute_email = mail
+
 #################################### Grafana.com Auth ####################
 [auth.grafana_com]
 ;enabled = false

diff --git a/docs/sources/auth/saml.md b/docs/sources/auth/saml.md
@@ -12,6 +12,8 @@ weight = 5
 
 # SAML Authentication
 
+> SAML Authentication integration is only available in Grafana Enterprise. Read more about [Grafana Enterprise]({{< relref "enterprise" >}}).
+
 > Only available in Grafana v6.3+
 
 The SAML authentication integration allows your Grafana users to log in by
@@ -36,6 +38,10 @@ At the moment of writing, Grafana supports:
 
     - `HTTP-POST` binding
 
+3. In terms of security, we currently support signed and encrypted Assertions. However, signed or encrypted requests are not supported.
+
+4. In terms of initiation, only SP-initiated requests are supported. There's no support for IdP-initiated request.
+
 ## Set up SAML Authentication
 
 To use the SAML integration, you need to enable SAML in the [main config file]({{< relref "installation/configuration.md" >}}).
@@ -71,6 +77,15 @@ max_issue_delay =
 
 # Duration, for how long the SP's metadata should be valid. Defaults to 48 hours
 metadata_valid_duration =
+
+# Friendly name or name of the attribute within the SAML assertion to use as the user's name
+assertion_attribute_name = displayName
+
+# Friendly name or name of the attribute within the SAML assertion to use as the user's login handle
+assertion_attribute_login = mail
+
+# Friendly name or name of the attribute within the SAML assertion to use as the user's email
+assertion_attribute_email = mail
 ```
 
 Important to note:
@@ -90,18 +105,24 @@ private_key_path = "/path/to/private_key.pem"
 metadata_path = "/my/metadata.xml"
 max_issue_delay = 90s
 metadata_valid_duration = 48h
+assertion_attribute_name = displayName
+assertion_attribute_login = mail
+assertion_attribute_email = mail
 ```
 
 And here is a comprehensive list of the options:
 
-| Setting                   | Required | Description                                                                    | Default |
-|---------------------------|----------|--------------------------------------------------------------------------------|---------|
-| `eanbled`                 | No       | Whenever SAML authentication is allowed                                        | `false` |
-| `certificate|_path`       | Yes      | Base64-encoded string or Path for the SP X.509 certificate                     |         |
-| `private_key|_path`       | Yes      | Base64-encoded string or Path for the SP private key                           |         |
-| `idp_metadata|_path|_url` | Yes      | Base64-encoded string, Path or URL for the IdP SAML metadata XML               |         |
-| `max_issue_delay`         | No       | Duration, since the IdP issued a response and the SP is allowed to process it  | `90s`   |
-| `metadata_valid_duration` | No       | Duration, for how long the SP's metadata should be valid                       | `48h`   |
+| Setting                                                     | Required | Description                                                                                        | Default       |
+| ----------------------------------------------------------- | -------- | -------------------------------------------------------------------------------------------------- | ------------- |
+| `enabled`                                                   | No       | Whenever SAML authentication is allowed                                                            | `false`       |
+| `certificate` or `certificate_path`                         | Yes      | Base64-encoded string or Path for the SP X.509 certificate                                         |               |
+| `private_key` or `private_key_path`                         | Yes      | Base64-encoded string or Path for the SP private key                                               |               |
+| `idp_metadata` or `idp_metadata_path` or `idp_metadata_url` | Yes      | Base64-encoded string, Path or URL for the IdP SAML metadata XML                                   |               |
+| `max_issue_delay`                                           | No       | Duration, since the IdP issued a response and the SP is allowed to process it                      | `90s`         |
+| `metadata_valid_duration`                                   | No       | Duration, for how long the SP's metadata should be valid                                           | `48h`         |
+| `assertion_attribute_name`                                  | No       | Friendly name or name of the attribute within the SAML assertion to use as the user's name         | `displayName` |
+| `assertion_attribute_login`                                 | No       | Friendly name or name of the attribute within the SAML assertion to use as the user's login handle | `mail`        |
+| `assertion_attribute_email`                                 | No       | Friendly name or name of the attribute within the SAML assertion to use as the user's email        | `mail`        |
 
 ### Cert and Private Key
 
@@ -117,7 +138,7 @@ Currently, we support three ways of specifying the IdP metadata. Without a suffi
 
 ### Max Issue Delay
 
-Prevention of SAML response replay attacks and internal clock skews between the SP (Grafana) and the IdP is covered. You can set a maximum amount of time between the IdP issuing a response and the SP (Grafana) processing it.
+Prevention of SAML response replay attacks and internal clock skews between the SP (Grafana), and the IdP is covered. You can set a maximum amount of time between the IdP issuing a response and the SP (Grafana) processing it.
 
 The configuration options is specified as a duration e.g. `max_issue_delay = 90s` or `max_issue_delay = 1h`
 
@@ -129,9 +150,23 @@ The configuration option is specified as a duration e.g. `metadata_valid_duratio
 
 ## Identity Provider (IdP) registration
 
-Finally, for the SAML integration to work correctly, you need to make the IdP aware of the SP. You need to provide the IdP with the SP's metadata. 
+For the SAML integration to work correctly, you need to make the IdP aware of the SP.
+
+The integration provides two key endpoints as part of Grafana:
+
+- The `/saml/metadata` endpoint. Which contains the SP's metadata. You can either download and upload it manually or make the IdP request it directly from the endpoint. Some providers name it Identifier or Entity ID.
+
+- The `/saml/acs` endpoint. Which is intended to receive the ACS (Assertion Customer Service) callback. Some providers name it SSO URL or Reply URL.
+
+## Assertion mapping
+
+During the SAML SSO authentication flow, we receive the ACS (Assertion Customer Service) callback. The callback contains all the relevant information of the user under authentication embedded in the SAML response. Grafana parses the response to create (or update) the user within its internal database.
+
+For Grafana to map the user information, it looks at the individual attributes within the assertion. You can think of these attributes as Key/Value pairs (although, they contain more information than that).
+
+Grafana provides configuration options that let you modify which keys to look at for these values. The data we need to create the user in Grafana is Name, Login handle, and email.
 
-Grafana provides an endpoint for such at `/saml/metadata`. You can either download the metadata and upload it manually, or make the IdP request it directly from the endpoint.
+An example is `assertion_attribute_name = "givenName"` where Grafana looks within the assertion for an attribute with a friendly name or name of `givenName`. Both, the friendly name (e.g. `givenName`) or the name (e.g. `urn:oid:2.5.4.42`) can be used interchangeably as the value for the configuration option.
 
 ## Troubleshooting
 

diff --git a/docs/sources/enterprise/_index.md b/docs/sources/enterprise/_index.md
@@ -29,6 +29,10 @@ With Grafana Enterprise you can set up synchronization between LDAP Groups and T
 
 Datasource permissions allow you to restrict query access to only specific Teams and Users. [Learn More]({{< relref "permissions/datasource_permissions.md" >}}).
 
+### SAML Authentication
+
+Enables your Grafana Enterprise users to authenticate with SAML. [Learn More]({{< relref "auth/saml.md" >}}).
+
 ### Premium Plugins
 
 With a Grafana Enterprise license you will get access to premium plugins, including:

diff --git a/packaging/docker/Dockerfile b/packaging/docker/Dockerfile
@@ -1,4 +1,4 @@
-ARG BASE_IMAGE=ubuntu:latest
+ARG BASE_IMAGE=ubuntu:18.04
 FROM ${BASE_IMAGE}
 
 ARG GRAFANA_TGZ="grafana-latest.linux-x64.tar.gz"
@@ -12,7 +12,7 @@ COPY ${GRAFANA_TGZ} /tmp/grafana.tar.gz
 # Change to tar xfzv to make tar print every file it extracts
 RUN mkdir /tmp/grafana && tar xfz /tmp/grafana.tar.gz --strip-components=1 -C /tmp/grafana
 
-ARG BASE_IMAGE=ubuntu:latest
+ARG BASE_IMAGE=ubuntu:18.04
 FROM ${BASE_IMAGE}
 
 ARG GF_UID="472"

diff --git a/packaging/docker/build.sh b/packaging/docker/build.sh
@@ -59,10 +59,10 @@ docker_tag_all () {
 	fi
 }
 
-docker_build "ubuntu:latest" "grafana-latest.linux-x64.tar.gz" "${_docker_repo}:${_grafana_version}"
+docker_build "ubuntu:18.04" "grafana-latest.linux-x64.tar.gz" "${_docker_repo}:${_grafana_version}"
 if [ $BUILD_FAST = "0" ]; then
-	docker_build "arm32v7/ubuntu:latest" "grafana-latest.linux-armv7.tar.gz" "${_docker_repo}-arm32v7-linux:${_grafana_version}"
-	docker_build "arm64v8/ubuntu:latest" "grafana-latest.linux-arm64.tar.gz" "${_docker_repo}-arm64v8-linux:${_grafana_version}"
+	docker_build "arm32v7/ubuntu:18.04" "grafana-latest.linux-armv7.tar.gz" "${_docker_repo}-arm32v7-linux:${_grafana_version}"
+	docker_build "arm64v8/ubuntu:18.04" "grafana-latest.linux-arm64.tar.gz" "${_docker_repo}-arm64v8-linux:${_grafana_version}"
 fi
 # Tag as 'latest' for official release; otherwise tag as grafana/grafana:master
 if echo "$_grafana_tag" | grep -q "^v"; then

diff --git a/pkg/api/login.go b/pkg/api/login.go
@@ -81,7 +81,7 @@ func tryOAuthAutoLogin(c *models.ReqContext) bool {
 	}
 	oauthInfos := setting.OAuthService.OAuthInfos
 	if len(oauthInfos) != 1 {
-		log.Warn("Skipping OAuth auto login because multiple OAuth providers are configured.")
+		log.Warn("Skipping OAuth auto login because multiple OAuth providers are configured")
 		return false
 	}
 	for key := range setting.OAuthService.OAuthInfos {
@@ -114,12 +114,16 @@ func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) Res
 	}
 
 	if err := bus.Dispatch(authQuery); err != nil {
+		e401 := Error(401, "Invalid username or password", err)
 		if err == login.ErrInvalidCredentials || err == login.ErrTooManyLoginAttempts {
-			return Error(401, "Invalid username or password", err)
+			return e401
 		}
 
+		// Do not expose disabled status,
+		// just show incorrect user credentials error (see #17947)
 		if err == login.ErrUserDisabled {
-			return Error(401, "User is disabled", err)
+			hs.log.Warn("User is disabled", "user", cmd.User)
+			return e401
 		}
 
 		return Error(500, "Error while trying to authenticate user", err)

diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go
@@ -191,8 +191,11 @@ func (hs *HTTPServer) OAuthLogin(ctx *m.ReqContext) {
 		return
 	}
 
+	// Do not expose disabled status,
+	// just show incorrect user credentials error (see #17947)
 	if cmd.Result.IsDisabled {
-		hs.redirectWithError(ctx, login.ErrUserDisabled)
+		oauthLogger.Warn("User is disabled", "user", cmd.Result.Login)
+		hs.redirectWithError(ctx, login.ErrInvalidCredentials)
 		return
 	}
 

diff --git a/pkg/infra/remotecache/redis_storage.go b/pkg/infra/remotecache/redis_storage.go
@@ -22,8 +22,12 @@ func parseRedisConnStr(connStr string) (*redis.Options, error) {
 	keyValueCSV := strings.Split(connStr, ",")
 	options := &redis.Options{Network: "tcp"}
 	for _, rawKeyValue := range keyValueCSV {
-		keyValueTuple := strings.Split(rawKeyValue, "=")
+		keyValueTuple := strings.SplitN(rawKeyValue, "=", 2)
 		if len(keyValueTuple) != 2 {
+			if strings.HasPrefix(rawKeyValue, "password") {
+				// don't log the password
+				rawKeyValue = "password******"
+			}
 			return nil, fmt.Errorf("incorrect redis connection string format detected for '%v', format is key=value,key=value", rawKeyValue)
 		}
 		connKey := keyValueTuple[0]