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

Basic auth extension #4823

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions distribution/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@
<argument>io.druid.extensions:druid-examples</argument>
<argument>-c</argument>
<argument>io.druid.extensions:simple-client-sslcontext</argument>
<argument>-c</argument>
<argument>io.druid.extensions:druid-basic-security</argument>
<argument>${druid.distribution.pulldeps.opts}</argument>
</arguments>
</configuration>
Expand Down
157 changes: 157 additions & 0 deletions docs/content/development/extensions-core/druid-basic-security.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
---
layout: doc_page
---

# Druid-Basic-Security
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please removes the hyphens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed hyphens


This extension adds:
- an Authenticator which supports [HTTP Basic authentication](https://en.wikipedia.org/wiki/Basic_access_authentication)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link to the docs for Authenticator and Authorizer (druid's auth.md) -- otherwise, if a user comes in to this page first, they will be lost.

- an Authorizer which implements basic role-based access control

Make sure to [include](../../operations/including-extensions.html) `druid-basic-security` as an extension.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The authenticator and authorizer should be less tied together, and the docs should mention that you can use them independently of each other.

The authenticator, if you want identity to be determined by http basic authentication.

The authorizer, if you want authorization to be done using a builtin RBAC system.



## Configuration


### Properties
|Property|Description|Default|required|
|--------|-----------|-------|--------|
|`druid.auth.basic.initialAdminPassword`|Password to assign when Druid automatically creates the default admin account. See [Default user accounts](#default-user-accounts) for more information.|"druid"|No|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't related to the authenticator or authorizer? That seems strange to me.

|`druid.auth.basic.initialInternalClientPassword`|Password to assign when Druid automatically creates the default admin account. See [Default user accounts](#default-user-accounts) for more information.|"druid"|No|

### Creating an Authenticator
```
druid.auth.authenticatorChain=["MyBasicAuthenticator"]

druid.auth.authenticator.MyBasicAuthenticator.type=basic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example should include all required properties (looks like there are some which are not here).

```

To use the Basic authenticator, add an authenticator with type `basic` to the authenticatorChain. The example above uses the name "MyBasicAuthenticator" for the Authenticator.

Configuration of the named authenticator is assigned through properties with the form:

```
druid.auth.authenticator.<authenticatorName>.<authenticatorProperty>
```

The configuration examples in the rest of this document will use "MyBasicAuthenticator" as the name of the authenticator being configured.

Only one instance of a "basic" type authenticator should be created and used, multiple "basic" authenticator instances are not supported.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you configure more than one?


#### Properties
|Property|Description|Default|required|
|--------|-----------|-------|--------|
|`druid.auth.authenticator.MyBasicAuthenticator.internalClientUsername`| Username for the internal system user, used for internal node communication|N/A|Yes|
|`druid.auth.authenticator.MyBasicAuthenticator.internalClientPassword`| Password for the internal system user, used for internal node communication|N/A|Yes|
|`druid.auth.authenticator.MyBasicAuthenticator.authorizerName`|Authorizer that requests should be directed to|N/A|Yes|

### Creating an Authorizer
```
druid.auth.authorizers=["MyBasicAuthorizer"]

druid.auth.authorizer.MyBasicAuthorizer.type=basic
```

To use the Basic authorizer, add an authenticator with type `basic` to the authorizers list. The example above uses the name "MyBasicAuthorizer" for the Authorizer.

Configuration of the named authenticator is assigned through properties with the form:

```
druid.auth.authorizer.<authorizerName>.<authorizerProperty>
```

The Basic authorizer has no additional configuration properties at this time.

Only one instance of a "basic" type authorizer should be created and used, multiple "basic" authorizer instances are not supported.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you configure more than one?



## Usage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usage doc should cover how to make queries using http basic authentication.



### Coordinator Security API
To use these APIs, a user needs read/write permissions for the CONFIG resource type with name "security".

Root path: `/druid/coordinator/v1/security`

#### User Management
`GET(/users)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include the prefix /druid/coordinator/v1/security in these API paths. Users generally scan docs rather than read the entire thing front to back, so it's important for stuff like the root prefix to be localized to the example.

Return a list of all user names.

`GET(/users/{userName})`
Return the name, roles, permissions of the user named {userName}

`POST(/users/{userName})`
Create a new user with name {userName}

`DELETE(/users/{userName})`
Delete the user with name {userName}


#### User Credentials
`GET(/credentials/{userName})`
Return the salt/hash/iterations info used for HTTP basic authentication for {userName}

`POST(/credentials/{userName})`
Assign a password used for HTTP basic authentication for {userName}
Content: password string


#### Role Creation/Deletion
`GET(/roles)`
Return a list of all role names.

`GET(/roles/{roleName})`
Return name and permissions for the role named {roleName}

`POST(/roles/{roleName})`
Create a new role with name {roleName}.
Content: username string

`DELETE(/roles/{roleName})`
Delete the role with name {roleName}.


#### Role Assignment
`POST(/users/{userName}/roles/{roleName})`
Assign role {roleName} to user {userName}.

`DELETE(/users/{userName}/roles/{roleName})`
Unassign role {roleName} from user {userName}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs should specify what will be returned in different situations.

Do you get JSON back? Do the HTTP codes mean anything? How does a caller know if the call succeeded or not?

This is useful for all APIs, although I thought of it specifically for DELETE since it's particularly unclear for that one. At least with POST Druid is pretty consistent about returning JSON and having HTTP status codes mean 2xx = your call succeeded; anything else = it didn't succeed.



#### Permissions
`POST(/roles/{roleName}/permissions)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think permissions should be first-class objects. API-wise, it would be simpler if they were just JSON lists on the payload of a role.

This would also allow us to have fewer tables, since we won't need a permission table or a role -> permission link table.

Create a new permissions and assign them to role named {roleName}.
Content: List of JSON Resource-Action objects, e.g.:
```
[
{
resource": {
"name": "wiki.*",
"type": "DATASOURCE"
},
"action": "READ"
},
{
resource": {
"name": "wikiticker",
"type": "DATASOURCE"
},
"action": "WRITE"
}
]
```

`DELETE(/permissions/{permId})`
Delete the permission with ID {permId}. Permission IDs are available from the output of individual user/role GET endpoints.

## Default user accounts

By default, an administrator account with full privileges is created with credentials `admin/druid`. The password assigned at account creation can be overridden by setting the `druid.auth.basic.initialAdminPassword` property.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two accounts should be optional as to whether they exist at all. It's because there might be other authenticators in play, and maybe there is no need for a basic admin or basic internal user.


A default internal system user account with full privileges, meant for internal communications between Druid services, is also created with credentials `druid_system/druid`. The password assigned at account creation can be overridden by setting the `druid.auth.basic.initialInternalClientPassword` property.

The values for `druid.authenticator.<authenticatorName>.internalClientUsername` and `druid.authenticator.<authenticatorName>.internalClientPassword` must match the credentials of the internal system user account.

Cluster administrators should change the default passwords for these accounts before exposing a cluster to users.
78 changes: 78 additions & 0 deletions extensions-core/druid-basic-security/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?xml version="1.0" encoding="UTF-8"?>

<!--
~ Druid - a distributed column store.
~ Copyright 2012 - 2015 Metamarkets Group Inc.
~
~ Licensed under the Apache License, Version 2.0 (the "License");
~ you may not use this file except in compliance with the License.
~ You may obtain a copy of the License at
~
~ http://www.apache.org/licenses/LICENSE-2.0
~
~ Unless required by applicable law or agreed to in writing, software
~ distributed under the License is distributed on an "AS IS" BASIS,
~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
~ See the License for the specific language governing permissions and
~ limitations under the License.
-->

<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>

<groupId>io.druid.extensions</groupId>
<artifactId>druid-basic-security</artifactId>
<name>druid-basic-security</name>
<description>druid-basic-security</description>

<parent>
<groupId>io.druid</groupId>
<artifactId>druid</artifactId>
<version>0.11.1-SNAPSHOT</version>
<relativePath>../../pom.xml</relativePath>
</parent>

<dependencies>
<dependency>
<groupId>io.druid</groupId>
<artifactId>druid-services</artifactId>
<version>${project.parent.version}</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>io.druid</groupId>
<artifactId>druid-server</artifactId>
<version>${project.parent.version}</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>mysql</groupId>
<artifactId>mysql-connector-java</artifactId>
<version>5.1.38</version>
Copy link
Contributor

@jihoonson jihoonson Oct 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mysql-metadata-storage also has this dependency of the same version. Did you intentionally specify these versions separately? Similar comment on the dependency on postgresql below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, moved the mysql/postgres versions to the main druid pom.xml, changed these to reference those versions

</dependency>
<dependency>
<groupId>org.postgresql</groupId>
<artifactId>postgresql</artifactId>
<version>9.4.1208.jre7</version>
</dependency>
<dependency>
<groupId>org.jdbi</groupId>
<artifactId>jdbi</artifactId>
<scope>provided</scope>
</dependency>

<!-- Tests -->
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.easymock</groupId>
<artifactId>easymock</artifactId>
<scope>test</scope>
</dependency>
</dependencies>
</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Licensed to Metamarkets Group Inc. (Metamarkets) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. Metamarkets licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package io.druid.security.basic;

import com.fasterxml.jackson.annotation.JsonProperty;

public class BasicAuthConfig
{
@JsonProperty
private String initialAdminPassword = "druid";

@JsonProperty
private String initialInternalClientPassword = "druid";

public String getInitialAdminPassword()
{
return initialAdminPassword;
}

public String getInitialInternalClientPassword()
{
return initialInternalClientPassword;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/*
* Licensed to Metamarkets Group Inc. (Metamarkets) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. Metamarkets licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package io.druid.security.basic;

import io.druid.java.util.common.StringUtils;
import io.druid.java.util.common.logger.Logger;

import javax.crypto.SecretKey;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.PBEKeySpec;
import javax.servlet.http.HttpServletRequest;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.security.spec.InvalidKeySpecException;
import java.util.Base64;

public class BasicAuthUtils
{
private static final Logger log = new Logger(BasicAuthUtils.class);
private static final Base64.Encoder ENCODER = Base64.getEncoder();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ENCODER and DECODER constants aren't that valuable since Base64.getEncoder() and Base64.getDecoder() already return constants.

private static final Base64.Decoder DECODER = Base64.getDecoder();
private static final SecureRandom SECURE_RANDOM = new SecureRandom();


public static int SALT_LENGTH = 32;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These constants should all be final.

Please also include some commentary on how they were chosen. Their values are important since they affect the security of the key derivation fn.

public static int KEY_ITERATIONS = 10000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to migrate to a higher number of iterations in the future without downtime? How would we do it?

public static int KEY_LENGTH = 512;
public static String ALGORITHM = "PBKDF2WithHmacSHA512";

public static String getEncodedCredentials(final String unencodedCreds)
{
return ENCODER.encodeToString(StringUtils.toUtf8(unencodedCreds));
}

public static byte[] hashPassword(final char[] password, final byte[] salt, final int iterations)
{
try {
SecretKeyFactory keyFactory = SecretKeyFactory.getInstance(ALGORITHM);
SecretKey key = keyFactory.generateSecret(
new PBEKeySpec(
password,
salt,
iterations,
KEY_LENGTH
)
);
return key.getEncoded();
}
catch (InvalidKeySpecException ikse) {
log.error("WTF? invalid keyspec");
throw new RuntimeException(ikse);
}
catch (NoSuchAlgorithmException nsae) {
log.error("%s not supported on this system.", ALGORITHM);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fine to embed this error message inside the exception. And then the chances of getting the useful message out to the user are higher (exception messages are sometimes reported upstream in "nicer" ways, like being emitted in alerts).

throw new RuntimeException(nsae, StringUtils.format("%s not supported on this system.", ALGORITHM));

Similar for the other catch block.

throw new RuntimeException(nsae);
}
}

public static byte[] generateSalt()
{
byte salt[] = new byte[SALT_LENGTH];
SECURE_RANDOM.nextBytes(salt);
return salt;
}

public static String getBasicUserSecretFromHttpReq(HttpServletRequest httpReq)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nullable return

{
try {
String authHeader = httpReq.getHeader("Authorization");

if (authHeader == null) {
return null;
}

if (!authHeader.substring(0, 6).equals("Basic ")) {
return null;
}

String encodedUserSecret = authHeader.substring(6);
return StringUtils.fromUtf8(DECODER.decode(encodedUserSecret));
}
catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern (try something, catch any exception, return null) can mask coding errors so I'd discourage it. Imagine what would happen if StringUtils.fromUtf8 had a bug where it sometimes threw an exception on certain strings. It would be really tough to track that down.

Instead, it's better to either:

  • catch much narrower exceptions, that are unlikely to be thrown due to coding errors (such as where you catch NoSuchAlgorithmException in another method)
  • don't use exceptions at all, but instead do things like check length of authHeader before calling substring on it.

In this case I think the latter is best.

return null;
}
}
}
Loading