brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aledsage <...@git.apache.org>
Subject [GitHub] incubator-brooklyn pull request: Jclouds tidy
Date Mon, 26 Jan 2015 11:21:35 GMT
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/465#discussion_r23523860
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java
---
    @@ -1305,118 +1326,148 @@ public UserCreation(LoginCredentials creds, List<Statement>
statements) {
          *   
          * @param image  The image being used to create the VM
          * @param config Configuration for creating the VM
    -     * @return       The commands required to create the user, along with the expected
login credentials.
    +     * @return       The commands required to create the user, along with the expected
login credentials for that user,
    +     * or null if we are just going to use those from jclouds.
          */
         protected UserCreation createUserStatements(@Nullable Image image, ConfigBag config)
{
    -        //NB: we ignore private key here because, by default we probably should not be
installing it remotely;
    -        //also, it may not be valid for first login (it is created before login e.g.
on amazon, so valid there;
    -        //but not elsewhere, e.g. on rackspace).
    +        //NB: private key is not installed remotely, just used to get/validate the public
key
             
    -        LoginCredentials loginCreds = null;
    +        LoginCredentials createdUserCreds = null;
             String user = getUser(config);
             String explicitLoginUser = config.get(LOGIN_USER);
             String loginUser = groovyTruth(explicitLoginUser) ? explicitLoginUser : (image
!= null && image.getDefaultCredentials() != null) ? image.getDefaultCredentials().identity
: null;
             Boolean dontCreateUser = config.get(DONT_CREATE_USER);
             Boolean grantUserSudo = config.get(GRANT_USER_SUDO);
    -        String publicKeyData = LocationConfigUtils.getPublicKeyData(config);
    -        String privateKeyData = LocationConfigUtils.getPrivateKeyData(config);
    -        String explicitPassword = config.get(PASSWORD);
    -        String password = groovyTruth(explicitPassword) ? explicitPassword : Identifiers.makeRandomId(12);
    +        OsCredential credential = LocationConfigUtils.getOsCredential(config);
    +        credential.checkNoErrors().logAnyWarnings();
    +        String passwordToSet = Strings.isNonBlank(credential.getPassword()) ? credential.getPassword()
: Identifiers.makeRandomId(12);
             List<Statement> statements = Lists.newArrayList();
             
             if (groovyTruth(dontCreateUser)) {
    -            // TODO For dontCreateUser, we probably only want to treat it special if
user was explicitly supplied
    -            // (rather than it just being the default config key value). If user was
explicit, then should
    -            // set the password + authorize the key for that user. Presumably the caller
knows that this
    -            // user pre-exists on the given VM image.
    +            // dontCreateUser:
    +            // if caller has not specified a user, we'll just continue to use the loginUser;
    +            // if caller *has*, we set up our credentials assuming that user and credentials
already exist
    +            
                 if (!groovyTruth(user)) {
    -                // loginCreds result will be null; use creds returned by jclouds on the
node
    +                // createdUserCreds returned from this method will be null; 
    +                // we will use the creds returned by jclouds on the node
                     LOG.info("Not setting up any user (subsequently using loginUser {})",
user, loginUser);
                     config.put(USER, loginUser);
                     
                 } else {
    -                LOG.info("Not creating user {}, and not setting its password or authorizing
keys", user);
    -                
    -                if (privateKeyData != null) {
    -                    loginCreds = LoginCredentials.builder().user(user).privateKey(privateKeyData).build();
    -                } else if (explicitPassword != null) {
    -                    loginCreds = LoginCredentials.builder().user(user).password(password).build();
    +                LOG.info("Not creating user {}, and not installing its password or authorizing
keys (assuming it exists)", user);
    +
    +                if (credential.isUsingPassword()) {
    +                    createdUserCreds = LoginCredentials.builder().user(user).password(credential.get()).build();
    +                } else if (credential.hasKey()) {
    +                    createdUserCreds = LoginCredentials.builder().user(user).privateKey(credential.get()).build();
                     }
                 }
                 
    -        } else if (!groovyTruth(user) || user.equals(loginUser)) {
    +        } else if (Strings.isBlank(user) || user.equals(loginUser) || user.equals(ROOT_USERNAME))
{
                 // For subsequent ssh'ing, we'll be using the loginUser
    -            if (!groovyTruth(user)) {
    -                config.put(USER, loginUser);
    +            if (Strings.isBlank(user)) {
    +                user = loginUser;
    +                config.put(USER, user);
                 }
     
                 // Using the pre-existing loginUser; setup the publicKey/password so can
login as expected
    -            if (password != null) {
    -                statements.add(new ReplaceShadowPasswordEntry(Sha512Crypt.function(),
loginUser, password));
    -                loginCreds = LoginCredentials.builder().user(loginUser).password(password).build();
    +            if (Strings.isNonBlank(passwordToSet)) {
    +                statements.add(new ReplaceShadowPasswordEntry(Sha512Crypt.function(),
user, passwordToSet));
    +                createdUserCreds = LoginCredentials.builder().user(user).password(passwordToSet).build();
                 }
    -            if (publicKeyData!=null) {
    -                statements.add(new AuthorizeRSAPublicKeys("~"+loginUser+"/.ssh", ImmutableList.of(publicKeyData)));
    -                if (privateKeyData != null) {
    -                    loginCreds = LoginCredentials.builder().user(loginUser).privateKey(privateKeyData).build();
    +            if (Strings.isNonBlank(credential.getPublicKeyData())) {
    +                statements.add(new AuthorizeRSAPublicKeys("~"+user+"/.ssh", ImmutableList.of(credential.getPublicKeyData())));
    +                if (!credential.isUsingPassword() && Strings.isNonBlank(credential.getPrivateKeyData()))
{
    +                    createdUserCreds = LoginCredentials.builder().user(user).privateKey(credential.getPrivateKeyData()).build();
                     }
                 }
                 
    -        } else if (user.equals(ROOT_USERNAME)) {
    -            // Authorizes the public-key and sets password for the root user, so can
login as expected
    -            if (password != null) {
    -                statements.add(new ReplaceShadowPasswordEntry(Sha512Crypt.function(),
ROOT_USERNAME, password));
    -                loginCreds = LoginCredentials.builder().user(user).password(password).build();
    -            }
    -            if (publicKeyData!=null) {
    -                statements.add(new AuthorizeRSAPublicKeys("~"+ROOT_USERNAME+"/.ssh",
ImmutableList.of(publicKeyData)));
    -                if (privateKeyData != null) {
    -                    loginCreds = LoginCredentials.builder().user(user).privateKey(privateKeyData).build();
    +        } else {
    +            String pubKey = credential.getPublicKeyData();
    +            String privKey = credential.getPrivateKeyData();
    +            
    +            if (credential.get()==null) {
    +                if (!loggedSshKeysHint && !config.containsKey(PRIVATE_KEY_FILE))
{
    +                    loggedSshKeysHint = true;
    +                    LOG.info("Default SSH keys not found or not usable; will create new
keys for each machine. "
    +                        + "Create ~/.ssh/id_rsa or "
    +                        + "set "+PRIVATE_KEY_FILE.getName()+" / "+PRIVATE_KEY_PASSPHRASE.getName()+"
/ "+PASSWORD.getName()+" "
    +                        + "as appropriate for this location if you wish to be able to
log in without Brooklyn.");
                     }
    +                KeyPair newKeyPair = SecureKeys.newKeyPair();
    --- End diff --
    
    I think that I'd prefer an explicit configuration option to enable creating new keys.
The question is what use-case this is covering.
    
    If it is for advanced users, and requires an explicit act to set the config value to blank,
then the user could equally well have set a more explicit config key to tell brooklyn to crate
the keypair.
    
    If it is for newbies that don't have ssh keys set up yet (and don't intend to), then I
think the automatic key-gen is a false economy for them. At some point they'll want to log
in, so would have to manually find the private key in the debug log and create an appropriately
permissioned file with it. That sounds more onerous than running a standard key-gen command
up front.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message