ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Hurley" <jhur...@hortonworks.com>
Subject Re: Review Request 38865: Create a credentials resource used to securely set, update, and remove credentials used by Ambari
Date Wed, 30 Sep 2015 15:20:50 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38865/#review101117
-----------------------------------------------------------

Ship it!


Big review :)

My preference is to use feature branches for this type of work so that you can make smaller,
more iterative contributions. Larger commits have more chances of having problems missed.
And often times, we rush to get the bare minimum tests in just so we could post a review.
Not saying that's what I see here - just an overall observation so that we can make reviews
less painful and easier to understand.


ambari-server/src/main/java/org/apache/ambari/server/api/services/CredentialService.java (line
59)
<https://reviews.apache.org/r/38865/#comment158439>

    I don't think that there should be a body on an @GET method.



ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java (line
1319)
<https://reviews.apache.org/r/38865/#comment158440>

    Doc.



ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java (line
1323)
<https://reviews.apache.org/r/38865/#comment158442>

    StringUtils.isEmpty()



ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java (line
1334)
<https://reviews.apache.org/r/38865/#comment158441>

    Doc.



ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java (line
1338)
<https://reviews.apache.org/r/38865/#comment158443>

    StringUtils.isEmpty()



ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java (line
1349)
<https://reviews.apache.org/r/38865/#comment158444>

    Doc.



ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java (line
1372)
<https://reviews.apache.org/r/38865/#comment158445>

    Doc.



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java
(lines 172 - 173)
<https://reviews.apache.org/r/38865/#comment158446>

    This is an interesting way to retrieve the resource provider. Any reason you use a factory
instead of instantiating a singleton? Seems like this controller class does it both ways.



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CredentialResourceProvider.java
(lines 61 - 83)
<https://reviews.apache.org/r/38865/#comment158447>

    Creating a new static anonymous class for each set is something I don't see often. Although
it should not maintain a reference to the parent since it's static, I'd rather no use java
compiler voodoo for something simple here. Maybe just a static block to add the properties
to a normal static Set?



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CredentialResourceProvider.java
(line 127)
<https://reviews.apache.org/r/38865/#comment158448>

    Null or also empty?



ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationFilter.java
(lines 67 - 68)
<https://reviews.apache.org/r/38865/#comment158450>

    Wouldn't this be taken care of by your `hasPermission(...);` calls inside of ClusterService
?



ambari-server/src/main/java/org/apache/ambari/server/security/credential/Credential.java (line
21)
<https://reviews.apache.org/r/38865/#comment158451>

    Doc



ambari-server/src/main/java/org/apache/ambari/server/security/encryption/AbstractCredentialStore.java
(lines 63 - 65)
<https://reviews.apache.org/r/38865/#comment158453>

    Concurrency issue here? What happens if two threads are updating the same keystore with
different credentials?



ambari-server/src/main/java/org/apache/ambari/server/security/encryption/AbstractCredentialStore.java
(line 157)
<https://reviews.apache.org/r/38865/#comment158454>

    AES as a default is fine, I think. Any reason we need to make this configurable to adhere
to international regulations on strength of encryption?



ambari-server/src/main/java/org/apache/ambari/server/security/encryption/CredentialStoreService.java
(line 120)
<https://reviews.apache.org/r/38865/#comment158449>

    Thank you for doc'ing this. However, I still do not like using Boolean objects for 3-way
values. It's too easy to accidentally unbox it and cause a NPE.


- Jonathan Hurley


On Sept. 30, 2015, 8:28 a.m., Robert Levas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38865/
> -----------------------------------------------------------
> 
> (Updated Sept. 30, 2015, 8:28 a.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley, John Speidel, and Robert Nettleton.
> 
> 
> Bugs: AMBARI-13214
>     https://issues.apache.org/jira/browse/AMBARI-13214
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Storage of the credentials is to be done using the existing _secure_ credentials provider
API which already exits within Ambari.  
> 
> Credential may be stored in either Ambari's persistent or temporary secure storage facilities.

> 
> # Testing capabilities
> 
> * Request 
> ```
> GET api/v1/clusters/{CLUSTER_NAME}
> ```
> 
> * Responses
> ```
> 200 OK
> {
>   ...
>   "credential_store_properties" : {
>     "storage.persistent" : "true",
>     "storage.temporary" : "true"
>   },
>   ...
> }
> ```
> 
> # Creating credentials
> 
> * Request 
> ```
> POST /api/v1/clusters/{CLUSTER_NAME}/credentials/{ALIAS}
> {
>   "Credential" : {
>     "principal" : "USERNAME",
>     "key" : "SECRET",
>     "persist" : true
>   }
> }
> 
> Where:
> ** principal:  the principal (or username) part of the credential to store
> ** key: the secret key part of the credential to store
> ** persist:  a boolean value indicating whether to store this credential in a persisted
(true) or temporary (false) secure credential store
> ```
> 
> * Responses
> ```
> 200 OK
> ```
> ```
> 400 Bad Request
> {
>   "status": 400,
>   "message": "Cannot persist credential in Ambari's secure credential store since secure
storage has not yet be configured.  Use ambari-server setup-security to enable this feature."
> }
> ```
> ```
> 403 Forbidden
> {
>   "status": 403,
>   "message": "You do not have permissions to access this resource."
> }
> ```
> 
> # Updating credentials
> 
> * Request
> ```
> PUT /api/v1/clusters/{CLUSTER_NAME}/credentials/{ALIAS}
> {
>   "Credential" : {
>     "principal" : "USERNAME",
>     "key" : "SECRET1",
>     "persist" : true
>   }
> }
> 
> Where:
> ** principal:  the principal (or username) part of the credential to store
> ** key: the secret key part of the credential to store
> ** persist:  a boolean value indicating whether to store this credential in a persisted
(true) or temporary (false) secure credential store
> ```
> 
> * Responses
> ```
> 200 OK
> ```
> ```
> 400 Bad Request
> {
>   "status": 400,
>   "message": "Cannot persist credential in Ambari's secure credential store since secure
storage has not yet be configured.  Use ambari-server setup-security to enable this feature."
> }
> ```
> ```
> 403 Forbidden
> {
>   "status": 403,
>   "message": "You do not have permissions to access this resource."
> }
> ```
> 
> # Removing credentials
> 
> * Request
> ```
> DELETE /api/v1/clusters/{CLUSTER_NAME}/credentials/{ALIAS}
> ```
> 
> * Responses
> ```
> 200 OK
> ```
> ```
> 404 Not Found
> {
>   "status": 404,
>   "message": "Not Found"
> }
> ```
> ```403 Forbidden
> {
>   "status": 403,
>   "message": "You do not have permissions to access this resource."
> }
> ```
> 
> # Listing credentials
> 
> * Request
> ```
> GET /api/v1/clusters/{CLUSTER_NAME}/credentials
> ```
> 
> * Responses 
> ```
> 200 OK
> {
>   "href" : "http://host:8080/api/v1/clusters/c1/credentials",
>   "items" : [
>     {
>       "href" : "http://host:8080/api/v1/clusters/c1/credentials/kdc.admin.credentials",
>       "Credential" : {
>         "alias" : "kdc.admin.credentials",
>         "cluster_name" : "c1"
>       }
>     },
>     {
>       "href" : "http://host:8080/api/v1/clusters/c1/credentials/service.admin.credentials",
>       "Credential" : {
>         "alias" : "service.admin.credentials",
>         "cluster_name" : "c1"
>       }
>     }
>   ]
> }
> ```
> ```
> 404 Not Found
> {
>   "status": 404,
>   "message": "Not Found"
> }
> ```
> ```
> 403 Forbidden
> {
>   "status": 403,
>   "message": "You do not have permissions to access this resource."
> }
> ```
> 
> # Retrieving credentials
> 
> * Request
> ```
> GET /api/v1/clusters/{CLUSTER_NAME}/credentials/{ALIAS}
> ```
> 
> * Responses 
> ```
> 200 OK
> {
>   "href" : "http://host:8080/api/v1/clusters/c1/credentials/kdc.admin.credentials",
>   "Credential" : {
>     "alias" : "kdc.admin.credentials",
>     "cluster_name" : "c1",
>     "persist" : true
>   }
> }
> ```
> ```
> 404 Not Found
> {
>   "status": 404,
>   "message": "Not Found"
> }
> ```
> ```
> 403 Forbidden
> {
>   "status": 403,
>   "message": "You do not have permissions to access this resource."
> }
> ```
> 
> 
> Diffs
> -----
> 
>   ambari-server/docs/api/v1/credential-create.md PRE-CREATION 
>   ambari-server/docs/api/v1/credential-delete.md PRE-CREATION 
>   ambari-server/docs/api/v1/credential-get.md PRE-CREATION 
>   ambari-server/docs/api/v1/credential-list.md PRE-CREATION 
>   ambari-server/docs/api/v1/credential-resources.md PRE-CREATION 
>   ambari-server/docs/api/v1/credential-update.md PRE-CREATION 
>   ambari-server/docs/api/v1/index.md c1e464c 
>   ambari-server/src/main/java/org/apache/ambari/server/api/resources/CredentialResourceDefinition.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java
1e219ff 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterService.java
7bb0a72 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/CredentialService.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
e3686ac 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java
6ba6bac 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/ClusterResponse.java
bb6d88e 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java
a40fae6 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java
a1cd5b8 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/ResourceProviderFactory.java
5d1143a 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java
9163656 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java
7e75a75 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CredentialResourceProvider.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Resource.java 1b208fb

>   ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationFilter.java
44c9613 
>   ambari-server/src/main/java/org/apache/ambari/server/security/credential/Credential.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/security/credential/CredentialFactory.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/security/credential/GenericKeyCredential.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/security/credential/InvalidCredentialValueException.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/security/credential/PrincipalKeyCredential.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/security/encryption/AbstractCredentialStore.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/security/encryption/CredentialProvider.java
b812337 
>   ambari-server/src/main/java/org/apache/ambari/server/security/encryption/CredentialStore.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/security/encryption/CredentialStoreService.java
4aa3b0a 
>   ambari-server/src/main/java/org/apache/ambari/server/security/encryption/CredentialStoreServiceImpl.java
968e96a 
>   ambari-server/src/main/java/org/apache/ambari/server/security/encryption/FileBasedCredentialStore.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/security/encryption/FileBasedCredentialStoreService.java
41ff71b 
>   ambari-server/src/main/java/org/apache/ambari/server/security/encryption/InMemoryCredentialStore.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/security/encryption/InMemoryCredentialStoreService.java
08d84fc 
>   ambari-server/src/test/java/org/apache/ambari/server/api/resources/CredentialResourceDefinitionTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/api/services/CredentialServiceTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
074fbb4 
>   ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java
23ce914 
>   ambari-server/src/test/java/org/apache/ambari/server/controller/internal/CredentialResourceProviderTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UserResourceProviderTest.java
b0e1018 
>   ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationFilterTest.java
1824486 
>   ambari-server/src/test/java/org/apache/ambari/server/security/encryption/CredentialProviderTest.java
ef1a9c8 
>   ambari-server/src/test/java/org/apache/ambari/server/security/encryption/CredentialStoreServiceImplTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/security/encryption/CredentialStoreServiceTest.java
9725746 
>   ambari-server/src/test/java/org/apache/ambari/server/security/encryption/CredentialStoreTest.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38865/diff/
> 
> 
> Testing
> -------
> 
> Units tests updated and passed
> Manually testing in existing cluster (upgrade scenario) and new cluster
> 
> # Local test results:
> 
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 54:46.952s
> [INFO] Finished at: Tue Sep 29 18:02:43 EDT 2015
> [INFO] Final Memory: 66M/1534M
> [INFO] ------------------------------------------------------------------------
> 
> # Jenkins test results: 
> 
> Tests run: 3231, Failures: 0, Errors: 0, Skipped: 25
> 
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 01:15 h
> [INFO] Finished at: 2015-09-30T04:19:21+00:00
> [INFO] Final Memory: 48M/564M
> [INFO] ------------------------------------------------------------------------
> 
> 
> Thanks,
> 
> Robert Levas
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message