Basic auth extension #4823
Basic auth extension #4823
Conversation
|
👍 |
|
@jon-wei please merge from master if you haven't recently; I hope that will fix the VM crash we got during CI. |
|
Making some modifications to this re: password initialization, marking WIP |
jon-wei
force-pushed
the
basic_sec
branch
2 times, most recently
from
d0c7577 to
e9ba4ff
Compare
| <dependency> | ||
| <groupId>mysql</groupId> | ||
| <artifactId>mysql-connector-java</artifactId> | ||
| <version>5.1.38</version> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hm, moved the mysql/postgres versions to the main druid pom.xml, changed these to reference those versions
| layout: doc_page | ||
| --- | ||
|
|
||
| # Druid-Basic-Security |
There was a problem hiding this comment.
Please removes the hyphens.
| import java.util.Map; | ||
| import java.util.concurrent.Callable; | ||
|
|
||
| public abstract class SQLBasicSecurityStorageConnector implements BasicSecurityStorageConnector |
There was a problem hiding this comment.
Please add some java doc and unit tests.
There was a problem hiding this comment.
Added javadoc and tests
| return dataSource; | ||
| } | ||
|
|
||
| protected boolean connectorIsTransientException(Throwable e) |
There was a problem hiding this comment.
This always returns false. Is this for any user-defined subclasses?
There was a problem hiding this comment.
I copied this from SQLMetadataConnector initially, I think that was the intent, this is now in the common BaseSQLMetadataConnector class
| * | ||
| * @return String representing the SQL type | ||
| */ | ||
| protected String getPayloadType() |
There was a problem hiding this comment.
Please remove unused method.
| @JsonTypeName("basic") | ||
| public class BasicRoleBasedAuthorizer implements Authorizer | ||
| { | ||
| private static final Logger log = new Logger(BasicRoleBasedAuthorizer.class); |
There was a problem hiding this comment.
Please remove unused variable.
| ) | ||
| { | ||
| if (authenticationResult == null) { | ||
| return new Access(false); |
There was a problem hiding this comment.
Any error message like 'null authenticationResult'?
There was a problem hiding this comment.
I changed this to throw an IAE, the authenticationResult should never be null here and indicates a bug if it is
| authenticationResult.getIdentity() | ||
| ); | ||
| if (authorizationName == null) { | ||
| return new Access(false); |
There was a problem hiding this comment.
Similar comment. 'Cannot find an authorization name for authenticationResult.getIdentity()'?
There was a problem hiding this comment.
I decided to remove the authentication name -> authorization name features, felt like the benefit (possibly avoiding cases where two user accounts are essentially the same with a different name) wasn't worth the added complexity, so this block is gone now
| } | ||
| } | ||
|
|
||
| return new Access(false); |
There was a problem hiding this comment.
Similar comment. Maybe 'permission denied'?
There was a problem hiding this comment.
hm, I think that's a bit redundant in this case, since Access(false) already directly expresses an authorization denial
| String permissionResourceName = permissionResource.getName(); | ||
| Pattern resourceNamePattern = Pattern.compile(permissionResourceName); | ||
| Matcher resourceNameMatcher = resourceNamePattern.matcher(resource.getName()); | ||
| return resourceNameMatcher.matches(); |
There was a problem hiding this comment.
Hmm, do you have any performance benchmark result? I'm not sure how this is fast enough.
There was a problem hiding this comment.
Not yet, I'll run some shortly
| ImmutableList.of( | ||
| StringUtils.format( | ||
| "CREATE TABLE %1$s (\n" | ||
| + " user_name INTEGER NOT NULL, \n" |
| ); | ||
| } | ||
| catch (IOException ioe) { | ||
| return null; |
There was a problem hiding this comment.
Maybe better to return a map containing the exception?
There was a problem hiding this comment.
Made this propagate the exception since it should never happen under any normal conditions
| public List<Map<String, Object>> inTransaction(Handle handle, TransactionStatus transactionStatus) | ||
| throws Exception | ||
| { | ||
| List<Map<String, Object>> role_permissions = handle |
There was a problem hiding this comment.
Should be the camel case or simply return without defining a variable.
There was a problem hiding this comment.
Changed this and similar places to return without defining a variable
| public List<Map<String, Object>> inTransaction(Handle handle, TransactionStatus transactionStatus) | ||
| throws Exception | ||
| { | ||
| List<Map<String, Object>> user_permissions = handle |
There was a problem hiding this comment.
Should be the camel case or simply return without defining a variable.
| if (existingMapping == null) { | ||
| handle.createStatement( | ||
| StringUtils.format( | ||
| "INSERT INTO %1$s (authentication_name, authorization_name) VALUES (:authenticationName, :authorizationName)", |
There was a problem hiding this comment.
This is method is deleted now since I removed the name mapping table
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public interface BasicSecurityStorageConnector |
There was a problem hiding this comment.
What do you think about making all non get* methods to return the result indicating success or failure? For example, delete* methods return the number of deleted rows. It may be useful for operators to reduce mistakes.
| } | ||
| } | ||
|
|
||
| public <T> T retryWithHandle( |
There was a problem hiding this comment.
Or maybe it's better to create a new super class which both SQLMetadataConnector and this class extend.
|
|
||
| import java.net.InetAddress; | ||
|
|
||
| public class DerbyAuthorizationStorage |
There was a problem hiding this comment.
Removed unused class, the Derby implementation now uses MetadataStorage
| log.info("Derby connector instantiated with metadata storage [%s].", this.storage.getClass().getName()); | ||
| } | ||
|
|
||
| public DerbySQLBasicSecurityStorageConnector( |
There was a problem hiding this comment.
It's not instantiated directly, but the class is bound in BasicSecurityDruidModule:
PolyBind.optionBinder(binder, Key.get(BasicSecurityStorageConnector.class))
.addBinding("derby")
.to(DerbySQLBasicSecurityStorageConnector.class)
.in(ManageLifecycle.class);
PolyBind.optionBinder(binder, Key.get(SQLBasicSecurityStorageConnector.class))
.addBinding("derby")
.to(DerbySQLBasicSecurityStorageConnector.class)
.in(ManageLifecycle.class);
| @Override | ||
| public boolean tableExists(Handle handle, String tableName) | ||
| { | ||
| // ensure database defaults to utf8, otherwise bail |
There was a problem hiding this comment.
"bail" is used here in the sense of "abandon what it's doing"
| ); | ||
| } | ||
|
|
||
| private int getUserCountInTransaction(Handle handle, String userName) |
There was a problem hiding this comment.
What does the "InTransaction" suffix mean in this and below methods?
There was a problem hiding this comment.
It's referring to how they're called from within the DB transactions, for example:
getDBI().inTransaction(
new TransactionCallback<Void>()
{
@Override
public Void inTransaction(Handle handle, TransactionStatus transactionStatus) throws Exception
{
There was a problem hiding this comment.
Thanks for the explanation. I was confused with its name. It sounds to me like getUserCount() is executed in a transaction. I think just getUserCount() is fine.
There was a problem hiding this comment.
Cool, I changed the names to remove "inTransaction"
|
@jihoonson I added a RegexMatchBenchmark for benchmarking regex pattern compilation and matching, these are the results I'm seeing on my macbook: Roughly, with the test regexes used in that benchmark (one for UUID strings, and a more complex one taken from Granularity), compile + match takes ~2us or lower. Deserializing a byte[] representation of a Pattern using DefaultObjectMapper is also slower than recompiling the pattern from the regex string. |
|
@jon-wei thanks for sharing the benchmark result. I was concerned with the performance because BasicRoleBasedAuthorizer.permissionCheck() always compile a new pattern for each permissionResourceName. This method may become slow as the number of resourceAction increases. Do you have any better idea? Maybe caching compiled patterns? |
|
@jihoonson I added a cache for the compiled regex patterns |
|
@jon-wei thanks for the quick fix! |
| # Druid Basic Security | ||
|
|
||
| This extension adds: | ||
| - an Authenticator which supports [HTTP Basic authentication](https://en.wikipedia.org/wiki/Basic_access_authentication) |
There was a problem hiding this comment.
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.
| ### 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| |
There was a problem hiding this comment.
This isn't related to the authenticator or authorizer? That seems strange to me.
|
|
||
| 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. |
There was a problem hiding this comment.
What happens if you configure more than one?
|
|
||
| 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. |
There was a problem hiding this comment.
What happens if you configure more than one?
|
|
||
| Cluster administrators should change the default passwords for these accounts before exposing a cluster to users. | ||
|
|
||
| ## Defining permissions |
There was a problem hiding this comment.
This section of the doc looks like it applies to all authorizers and should be part of Druid's auth.md.
|
|
||
| String permissionResourceName = permissionResource.getName(); | ||
|
|
||
| Pattern resourceNamePattern = permissionPatternCache.get(permissionResourceName); |
There was a problem hiding this comment.
The permissionPatternCache won't be needed if all permissions details are cached -- since the permission objects would be long-lived and could contain their own compiled Patterns.
| return null; | ||
| } | ||
|
|
||
| if (dbConnector.checkCredentials(dbConfig.getDbPrefix(), user, password.toCharArray())) { |
There was a problem hiding this comment.
Same comment as in the Authorizer: we need to avoid hitting the DB here.
| String user = splits[0]; | ||
| char[] password = splits[1].toCharArray(); | ||
|
|
||
| if (dbConnector.checkCredentials(dbConfig.getDbPrefix(), user, password)) { |
There was a problem hiding this comment.
Similar -- need to avoid hitting the DB.
| createUserTable(dbConfig.getDbPrefix()); | ||
| createUserCredentialsTable(dbConfig.getDbPrefix()); | ||
|
|
||
| makeDefaultSuperuser( |
There was a problem hiding this comment.
IMO, this should be optional -- superuser may use a different authenticator.
| dbConfig.getInitialAdminPassword() | ||
| ); | ||
|
|
||
| makeDefaultSuperuser( |
There was a problem hiding this comment.
IMO, this should be optional -- internal may use a different authenticator.
|
closing this in favor of #5099 |
This PR adds an extension that provides implementations of the auth interfaces introduced in #4271 :