You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by dskarbek <gi...@git.apache.org> on 2016/12/09 23:10:43 UTC
[GitHub] qpid-dispatch pull request #124: Allow passing the password via env variable
GitHub user dskarbek opened a pull request:
https://github.com/apache/qpid-dispatch/pull/124
Allow passing the password via env variable
Allows for the password in the config to actually be the name of the environment variable that contains the password. This is indicated by prefixing the value with "env:". We also add the option to prefix with "literal:" which indicates that it is a raw password. That way, you can still give "env:SHELL" as a password, if that's what you really want to use, you would just have to list it as "literal:env:SHELL" in the config.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dskarbek/qpid-dispatch allow_password_in_env_var
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/qpid-dispatch/pull/124.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #124
----
commit f84a135084de49f900a845ff934e212b03ec7957
Author: Daniel Skarbek <ds...@paypal.com>
Date: 2016-12-09T01:02:39Z
Allow the password config to point to an environment variable with the actual password
----
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #124: Allow passing the password via env variable
Posted by dskarbek <gi...@git.apache.org>.
Github user dskarbek commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/124#discussion_r91815006
--- Diff: src/connection_manager.c ---
@@ -176,6 +176,46 @@ static void set_config_host(qd_server_config_t *config, qd_entity_t* entity)
assert(config->host);
}
+static char* qd_config_ssl_profile_get_password(qd_config_ssl_profile_t* ssl_profile)
+{
+ char *pw = ssl_profile->ssl_password;
+ if (!pw) return pw;
+
+ /* if the "password" starts with "env:" or "env: " then the remaining
+ * text is the environment variable that contains the password
+ */
+ if (strncmp(pw, "env:", 4) == 0) {
+ char *env = pw + 4;
+ /* skip the space if it is there */
+ if (*env == ' ') ++env;
+
+ const char* passwd = getenv(env);
+ if (passwd) {
+ free(ssl_profile->ssl_password);
+ pw = ssl_profile->ssl_password = strdup(passwd);
+ } else {
+ qd_error(QD_ERROR_NOT_FOUND, "Failed to find a password in the environment variable '%s'", env);
--- End diff --
or, would you rather just return the password string as is and not register an error in-case it is an actual password? If it is referring to an env var that missed getting set there's next to zero chance it will work, so better to catch the error sooner at the source so we can log it appropriately, but this does mean that someone using "env:foobar" as a password will suddenly stop working.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #124: Allow passing the password via env variable
Posted by dskarbek <gi...@git.apache.org>.
Github user dskarbek commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/124#discussion_r92076465
--- Diff: src/connection_manager.c ---
@@ -176,6 +176,46 @@ static void set_config_host(qd_server_config_t *config, qd_entity_t* entity)
assert(config->host);
}
+static char* qd_config_ssl_profile_get_password(qd_config_ssl_profile_t* ssl_profile)
+{
+ char *pw = ssl_profile->ssl_password;
+ if (!pw) return pw;
+
+ /* if the "password" starts with "env:" or "env: " then the remaining
+ * text is the environment variable that contains the password
+ */
+ if (strncmp(pw, "env:", 4) == 0) {
+ char *env = pw + 4;
+ /* skip the space if it is there */
+ if (*env == ' ') ++env;
+
+ const char* passwd = getenv(env);
+ if (passwd) {
+ free(ssl_profile->ssl_password);
+ pw = ssl_profile->ssl_password = strdup(passwd);
+ } else {
+ qd_error(QD_ERROR_NOT_FOUND, "Failed to find a password in the environment variable '%s'", env);
--- End diff --
Oh, you mean for the case where that isn't an env variable, but actually is part of a password? Hmm, that's true. though, it seems like it would help you understand what is happening faster if we log it. Is the log file less secure than the config file? Cuz, in that case, they already have the password in the config file.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #124: Allow passing the password via env variable
Posted by ted-ross <gi...@git.apache.org>.
Github user ted-ross commented on the issue:
https://github.com/apache/qpid-dispatch/pull/124
Dan,
I committed a modified version of this patch yesterday. Instead of
processing the password at connector/listener setup time, it does it at
ssl_profile setup time. This ensures that the password is interpreted
only once, eliminating the possible issues of re-processing.
-Ted
On 12/12/2016 07:53 PM, Daniel Skarbek wrote:
> dskarbek commented on this pull request.
>
>
>
>> +{
> + char *pw = ssl_profile->ssl_password;
> + if (!pw) return pw;
> +
> + /* if the "password" starts with "env:" or "env: " then the remaining
> + * text is the environment variable that contains the password
> + */
> + if (strncmp(pw, "env:", 4) == 0) {
> + char *env = pw + 4;
> + /* skip the space if it is there */
> + if (*env == ' ') ++env;
> +
> + const char* passwd = getenv(env);
> + if (passwd) {
> + free(ssl_profile->ssl_password);
> + pw = ssl_profile->ssl_password = strdup(passwd);
>
> Yes, this is replacing the value, and true, could cause problems if the literal value started with "env:". Actually, the fix you mention would not be so great. On the first time through the "literal" may or may not be handled depending on a subtle change in the code, and on the 3rd time, might have trouble or not. I think that the "env:" and "literal:" keywords are supposed to be in-effect on the config file value, and should not apply to the env-var value. That keeps it cleaner and simpler. So, that would mean that I shouldn't re-write the ssl_profile value. What do you think of just assigning the result of getenv() directly? Can we change the ssl_password member to be a const char*? Otherwise, I'd have to cast away const.
>
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #124: Allow passing the password via env variable
Posted by ted-ross <gi...@git.apache.org>.
Github user ted-ross commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/124#discussion_r92030610
--- Diff: src/connection_manager.c ---
@@ -176,6 +176,46 @@ static void set_config_host(qd_server_config_t *config, qd_entity_t* entity)
assert(config->host);
}
+static char* qd_config_ssl_profile_get_password(qd_config_ssl_profile_t* ssl_profile)
+{
+ char *pw = ssl_profile->ssl_password;
+ if (!pw) return pw;
+
+ /* if the "password" starts with "env:" or "env: " then the remaining
+ * text is the environment variable that contains the password
+ */
+ if (strncmp(pw, "env:", 4) == 0) {
+ char *env = pw + 4;
+ /* skip the space if it is there */
+ if (*env == ' ') ++env;
+
+ const char* passwd = getenv(env);
+ if (passwd) {
+ free(ssl_profile->ssl_password);
+ pw = ssl_profile->ssl_password = strdup(passwd);
+ } else {
+ qd_error(QD_ERROR_NOT_FOUND, "Failed to find a password in the environment variable '%s'", env);
--- End diff --
This should definitely be logged. It might be prudent to not include the password text in the log though.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #124: Allow passing the password via env variable
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/qpid-dispatch/pull/124
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #124: Allow passing the password via env variable
Posted by dskarbek <gi...@git.apache.org>.
Github user dskarbek commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/124#discussion_r91815078
--- Diff: src/connection_manager.c ---
@@ -176,6 +176,46 @@ static void set_config_host(qd_server_config_t *config, qd_entity_t* entity)
assert(config->host);
}
+static char* qd_config_ssl_profile_get_password(qd_config_ssl_profile_t* ssl_profile)
+{
+ char *pw = ssl_profile->ssl_password;
+ if (!pw) return pw;
+
+ /* if the "password" starts with "env:" or "env: " then the remaining
+ * text is the environment variable that contains the password
+ */
+ if (strncmp(pw, "env:", 4) == 0) {
+ char *env = pw + 4;
+ /* skip the space if it is there */
+ if (*env == ' ') ++env;
+
+ const char* passwd = getenv(env);
+ if (passwd) {
+ free(ssl_profile->ssl_password);
+ pw = ssl_profile->ssl_password = strdup(passwd);
+ } else {
+ qd_error(QD_ERROR_NOT_FOUND, "Failed to find a password in the environment variable '%s'", env);
+ }
+ return pw;
+ }
+
+ /* if the "password" starts with "literal:" or "literal: " then
+ * the remaining text is the password and the heading should be
+ * stripped off
+ */
+ if (strncmp(pw, "literal:", 8) == 0) {
+ /* skip the "literal:" header */
+ pw += 8;
+ /* skip the space if it is there */
+ if (*pw == ' ') ++pw;
+ /* return a pointer into the middle of the string where the literal password starts */
+ return pw;
--- End diff --
Not a problem unless I'm wrong about the memory management of this pointer.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #124: Allow passing the password via env variable
Posted by dskarbek <gi...@git.apache.org>.
Github user dskarbek commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/124#discussion_r91814838
--- Diff: src/connection_manager.c ---
@@ -176,6 +176,46 @@ static void set_config_host(qd_server_config_t *config, qd_entity_t* entity)
assert(config->host);
}
+static char* qd_config_ssl_profile_get_password(qd_config_ssl_profile_t* ssl_profile)
+{
+ char *pw = ssl_profile->ssl_password;
+ if (!pw) return pw;
+
+ /* if the "password" starts with "env:" or "env: " then the remaining
+ * text is the environment variable that contains the password
+ */
+ if (strncmp(pw, "env:", 4) == 0) {
+ char *env = pw + 4;
+ /* skip the space if it is there */
+ if (*env == ' ') ++env;
+
+ const char* passwd = getenv(env);
+ if (passwd) {
+ free(ssl_profile->ssl_password);
+ pw = ssl_profile->ssl_password = strdup(passwd);
--- End diff --
Hmm, or, if I'm correct that the config->ssl_password is never directly free'd, then I could just pass back the static getenv() returned string as the value (except I'd have to cast away const). Then I don't have to change the string held in ssl_profile. Thoughts? Is my memory management correct here (and below) that the config->ssl_password is a reference to memory that it doesn't own and free is not called on it?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #124: Allow passing the password via env variable
Posted by dskarbek <gi...@git.apache.org>.
Github user dskarbek commented on the issue:
https://github.com/apache/qpid-dispatch/pull/124
Ok, that makes sense. Thanks.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org