Mailing-List: contact dev-help@brooklyn.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@brooklyn.incubator.apache.org
From: aledsage <git@git.apache.org>
To: dev@brooklyn.incubator.apache.org
Reply-To: dev@brooklyn.incubator.apache.org
References: <git-pr-465-incubator-brooklyn@git.apache.org>
In-Reply-To: <git-pr-465-incubator-brooklyn@git.apache.org>
Subject: [GitHub] incubator-brooklyn pull request: Jclouds tidy
Content-Type: text/plain
Message-Id: <20150123174144.9BBE5E03A8@git1-us-west.apache.org>
Date: Fri, 23 Jan 2015 17:41:44 +0000 (UTC)

Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/465#discussion_r23466090
  
    --- Diff: core/src/main/java/brooklyn/location/basic/LocationConfigUtils.java ---
    @@ -20,39 +20,355 @@
     
     import static brooklyn.util.JavaGroovyEquivalents.groovyTruth;
     
    +import java.io.ByteArrayInputStream;
     import java.io.File;
    -import java.io.IOException;
    +import java.security.KeyPair;
    +import java.security.PublicKey;
     import java.util.Arrays;
    +import java.util.Iterator;
     import java.util.List;
     import java.util.Map;
    +import java.util.Set;
     
     import org.slf4j.Logger;
     import org.slf4j.LoggerFactory;
     
     import brooklyn.config.ConfigKey;
     import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.internal.BrooklynFeatureEnablement;
    +import brooklyn.location.cloud.CloudLocationConfig;
     import brooklyn.management.ManagementContext;
    +import brooklyn.util.ResourceUtils;
     import brooklyn.util.collections.MutableMap;
    +import brooklyn.util.collections.MutableSet;
     import brooklyn.util.config.ConfigBag;
    +import brooklyn.util.crypto.AuthorizedKeysParser;
    +import brooklyn.util.crypto.SecureKeys;
    +import brooklyn.util.crypto.SecureKeys.PassphraseProblem;
    +import brooklyn.util.exceptions.Exceptions;
     import brooklyn.util.os.Os;
     import brooklyn.util.text.StringFunctions;
     import brooklyn.util.text.Strings;
     
    -import com.google.common.base.Charsets;
     import com.google.common.base.Objects;
     import com.google.common.collect.ImmutableList;
     import com.google.common.collect.Iterables;
     import com.google.common.collect.Lists;
    -import com.google.common.io.Files;
     
     public class LocationConfigUtils {
     
         private static final Logger log = LoggerFactory.getLogger(LocationConfigUtils.class);
    +
    +    /** Creates an instance of {@link OsCredential} by inspecting {@link LocationConfigKeys#PASSWORD}; 
    +     * {@link LocationConfigKeys#PRIVATE_KEY_DATA} and {@link LocationConfigKeys#PRIVATE_KEY_FILE};
    +     * {@link LocationConfigKeys#PRIVATE_KEY_PASSPHRASE} if needed, and
    +     * {@link LocationConfigKeys#PRIVATE_KEY_DATA} and {@link LocationConfigKeys#PRIVATE_KEY_FILE}
    +     * (defaulting to the private key file + ".pub"). 
    +     **/
    +    public static OsCredential getOsCredential(ConfigBag config) {
    +        return OsCredential.newInstance(config);
    +    }
         
    +    /** Convenience class for holding private/public keys and passwords, inferring from config keys.
    +     * See {@link LocationConfigUtils#getOsCredential(ConfigBag)}. */
    +    public static class OsCredential {
    +        private final ConfigBag config;
    +        private boolean preferPassword = false;
    +        private boolean tryDefaultKeys = true;
    +        private boolean requirePublicKey = true;
    +        private boolean doKeyValidation = BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_VALIDATE_LOCATION_SSH_KEYS);
    +        private boolean warnOnErrors = true;
    +        private boolean throwOnErrors = false;
    +        
    +        private boolean dirty = true;;
    +        
    +        private String privateKeyData;
    +        private String publicKeyData;
    +        private String password;
    +        
    +        private OsCredential(ConfigBag config) {
    +            this.config = config;
    +        }
    +
    +        /** throws if there are any problems */
    +        public OsCredential checkNotEmpty() {
    +            checkNoErrors();
    +            
    +            if (!hasKey() && !hasPassword()) {
    +                if (warningMessages.size()>0)
    +                    throw new IllegalStateException("Could not find credentials: "+warningMessages);
    +                else 
    +                    throw new IllegalStateException("Could not find credentials");
    +            }
    +            return this;
    +        }
    +
    +        /** throws if there were errors resolving (e.g. explicit keys, none of which were found/valid, or public key required and not found) 
    +         * @return */
    +        public OsCredential checkNoErrors() {
    +            throwOnErrors(true);
    +            dirty();
    +            infer();
    +            return this;
    +        }
    +        
    +        public OsCredential logAnyWarnings() {
    +            if (!warningMessages.isEmpty())
    +                log.warn("When reading credentials: "+warningMessages);
    +            return this;
    +        }
    +
    +        public Set<String> getWarningMessages() {
    +            return warningMessages;
    +        }
    +        
    +        /** returns either the key or password or null; if both a key and a password this prefers the key unless otherwise set
    +         * via {@link #preferPassword()} */
    +        public synchronized String get() {
    +            infer();
    +            
    +            if (isUsingPassword()) return password;
    +            if (hasKey()) return privateKeyData;
    +            return null;
    +        }
    +
    +        /** if there is no credential (ignores public key) */
    +        public boolean isEmpty() {
    +            return !hasKey() && !hasPassword();
    +        }
    +        public boolean hasKey() {
    +            infer();
    +            // key has stricter non-blank check than password
    +            return Strings.isNonBlank(privateKeyData);
    +        }
    +        public boolean hasPassword() {
    +            infer();
    +            // blank, even empty passwords are allowed
    +            return password!=null;
    +        }
    +        /** if a password is available, and either this is preferred over a key or there is no key */
    +        public boolean isUsingPassword() {
    +            return hasPassword() && (!hasKey() || preferPassword);
    +        }
    +        
    +        public String getPrivateKeyData() {
    +            infer();
    +            return privateKeyData;
    +        }
    +        public String getPublicKeyData() {
    +            infer();
    +            return publicKeyData;
    +        }
    +        public String getPassword() {
    +            infer();
    +            return password;
    +        }
    +        
    +        /** if both key and password supplied, prefer the key; the default */
    +        public OsCredential preferKey() { preferPassword = false; return dirty(); }
    +        /** if both key and password supplied, prefer the password; see {@link #preferKey()} */
    +        public OsCredential preferPassword() { preferPassword = true; return dirty(); }
    +        
    +        /** if false, do not mind if there is no public key corresponding to any private key;
    +         * defaults to true; only applies if a private key is set */
    +        public OsCredential requirePublicKey(boolean requirePublicKey) {
    +            this.requirePublicKey = requirePublicKey;
    +            return dirty(); 
    +        }
    +        /** whether to check the private/public keys and passphrase are coherent; default true */
    +        public OsCredential doKeyValidation(boolean doKeyValidation) {
    +            this.doKeyValidation = doKeyValidation;
    +            return dirty();
    +        }
    +        /** if true (the default) this will look at default locations set on keys */
    +        public OsCredential useDefaultKeys(boolean tryDefaultKeys) {
    +            this.tryDefaultKeys = tryDefaultKeys;
    +            return dirty(); 
    +        }
    +        /** whether to log warnings on problems */
    +        public OsCredential warnOnErrors(boolean warnOnErrors) {
    +            this.warnOnErrors = warnOnErrors;
    +            return dirty(); 
    +        }
    +        /** whether to throw on problems */
    +        public OsCredential throwOnErrors(boolean throwOnErrors) {
    +            this.throwOnErrors = throwOnErrors;
    +            return dirty(); 
    +        }
    +        
    +        private OsCredential dirty() { dirty = true; return this; }
    +            
    +        public static OsCredential newInstance(ConfigBag config) {
    +            return new OsCredential(config);
    +        }
    +        
    +        private synchronized void infer() {
    +            if (!dirty) return;
    +            warningMessages.clear(); 
    +            
    +            log.debug("Inferring OS credentials");
    +            privateKeyData = config.get(LocationConfigKeys.PRIVATE_KEY_DATA);
    +            password = config.get(LocationConfigKeys.PASSWORD);
    +            publicKeyData = getKeyDataFromDataKeyOrFileKey(config, LocationConfigKeys.PUBLIC_KEY_DATA, LocationConfigKeys.PUBLIC_KEY_FILE);
    +
    +            KeyPair privateKey = null;
    +            
    +            if (Strings.isBlank(privateKeyData)) {
    +                // look up private key files
    +                String privateKeyFiles = null;
    +                boolean privateKeyFilesExplicitlySet = config.containsKey(LocationConfigKeys.PRIVATE_KEY_FILE);
    +                if (privateKeyFilesExplicitlySet || (tryDefaultKeys && password==null)) 
    +                    privateKeyFiles = config.get(LocationConfigKeys.PRIVATE_KEY_FILE);
    +                if (Strings.isNonBlank(privateKeyFiles)) {
    +                    Iterator<String> fi = Arrays.asList(privateKeyFiles.split(File.pathSeparator)).iterator();
    --- End diff --
    
    Maybe agree, but am hesitant for us to have different formats for the contents of brooklyn.properties on Windows and Unix.
    
    But the value of this field will look very different on linux and windows, given that it normally contains file paths.
    
    I see we use `getResourceAsString(file)` which means it would actually work with a URL (except for `:` having been used as a separator). I don't think we need to support URLs; the config key's description is clear that they are files.


---
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.
---