Work with variable references as long as possible (commands promises)

cf-agent/verify_exec.c

@@ -43,6 +43,7 @@
 #include <scope.h>
 #include <ornaments.h>
 #include <eval_context.h>
+#include <expand.h>
 #include <retcode.h>
 #include <timeout.h>
 
@@ -202,6 +203,33 @@ static char *GetLockNameExec(const Attributes *a, const Promise *pp)
 
 /*****************************************************************************/
 
+/**
+ * Resolve the secret references the promiser of a commands promise carries
+ * unexpanded (PromiseTypeResolvesSecretsAtUse() in promises.c).
+ *
+ * Every result goes straight to the process being started; it must not reach a
+ * log message, a lock name or a report.
+ *
+ * @return A new string, always; the caller owns it.
+ */
+static char *ResolveSecrets(const EvalContext *ctx, const char *ns,
+                            const char *string)
+{
+    assert(string != NULL);
+    if (string == NULL)
+    {
+        return NULL;
+    }
+
+    /* Nothing was deferred if no reference is left, the usual case. */
+    if (!IsCf3VarString(string))
+    {
+        return xstrdup(string);
+    }
+
+    return ExpandScalarSecretsOnly(ctx, ns, "this", string, NULL);
+}
+
 static ActionResult RepairExec(EvalContext *ctx, const Attributes *a,
                                const Promise *pp, PromiseResult *result)
 {
@@ -221,9 +249,22 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a,
 
     module_context[0] = '\0';
 
-    if (IsAbsoluteFileName(CommandArg0(pp->promiser)) || a->contain.shelltype == SHELL_TYPE_NONE)
+    const char *const ns = PromiseGetNamespace(pp);
+
+    /* "$(x)" never names an executable, so this check needs the values; the
+     * diagnostics below keep pp->promiser, which does not have them. Freed at
+     * once, and fetched again as late as possible at cf_popen(). CommandArg0()
+     * returns a static buffer, hence the copy. */
+    char *real_arg0;
+    {
+        char *const real_promiser = ResolveSecrets(ctx, ns, pp->promiser);
+        real_arg0 = xstrdup(CommandArg0(real_promiser));
+        free(real_promiser);
+    }
+
+    if (IsAbsoluteFileName(real_arg0) || a->contain.shelltype == SHELL_TYPE_NONE)
     {
-        if (!IsExecutable(CommandArg0(pp->promiser)))
+        if (!IsExecutable(real_arg0))
         {
             cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, "'%s' promises to be executable but isn't", pp->promiser);
             *result = PromiseResultUpdate(*result, PROMISE_RESULT_FAIL);
@@ -233,13 +274,15 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a,
                 Log(LOG_LEVEL_VERBOSE, "Paths with spaces must be inside escaped quoutes (e.g. \\\"%s\\\")", pp->promiser);
             }
 
+            free(real_arg0);
             return ACTION_RESULT_FAILED;
         }
         else
         {
             Log(LOG_LEVEL_VERBOSE, "Promiser string contains a valid executable '%s' - ok", CommandArg0(pp->promiser));
         }
     }
+    free(real_arg0);
 
     char timeout_str[CF_BUFSIZE];
     if (a->contain.timeout == CF_NOINT)
@@ -315,39 +358,52 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a,
         }
 #endif /* !__MINGW32__ */
 
+        /* Point of use: nothing below cf_popen*() needs the values, so the
+         * strings holding them are built and freed here. 'cmdline' keeps the
+         * references and is what every log message in this function prints. */
         const char *open_mode = a->module ? "rt" : "r";
         if (a->contain.shelltype == SHELL_TYPE_POWERSHELL)
         {
 #ifdef __MINGW32__
+            char *const real_cmdline = ResolveSecrets(ctx, ns, cmdline);
             pfp =
-                cf_popen_powershell_setuid(cmdline, open_mode, a->contain.owner, a->contain.group, a->contain.chdir, a->contain.chroot,
+                cf_popen_powershell_setuid(real_cmdline, open_mode, a->contain.owner, a->contain.group, a->contain.chdir, a->contain.chroot,
                                            a->transaction.background);
+            free(real_cmdline);
 #else // !__MINGW32__
             Log(LOG_LEVEL_ERR, "Powershell is only supported on Windows");
             return ACTION_RESULT_FAILED;
 #endif // !__MINGW32__
         }
         else if (a->contain.shelltype == SHELL_TYPE_USE)
         {
+            char *const real_cmdline = ResolveSecrets(ctx, ns, cmdline);
             pfp =
-                cf_popen_shsetuid(cmdline, open_mode, a->contain.owner, a->contain.group, a->contain.chdir, a->contain.chroot,
+                cf_popen_shsetuid(real_cmdline, open_mode, a->contain.owner, a->contain.group, a->contain.chdir, a->contain.chroot,
                                   a->transaction.background);
+            free(real_cmdline);
         }
         else
         {
             char *const command = (a->args == NULL)
                                 ? xstrdup(pp->promiser)
                                 : StringFormat("%s %s", pp->promiser,
                                                a->args);
+            char *const real_command = ResolveSecrets(ctx, ns, command);
+            free(command);
+
+            /* arglist entries are ordinary constraints, already expanded, but
+             * one can hold $(this.promiser), which here is unexpanded. */
             Seq *arglist = NULL;
             if (a->arglist != NULL)
             {
-                arglist = SeqNew(8, NULL);
+                arglist = SeqNew(RlistLen(a->arglist), free);
                 for (const Rlist *rp = a->arglist; rp != NULL; rp = rp->next)
                 {
                     if (rp->val.type == RVAL_TYPE_SCALAR)
                     {
-                        SeqAppend(arglist, RlistScalarValue(rp));
+                        SeqAppend(arglist,
+                                  ResolveSecrets(ctx, ns, RlistScalarValue(rp)));
                     }
                     else
                     {
@@ -357,11 +413,11 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a,
                     }
                 }
             }
-            pfp = cf_popensetuid(command, arglist, open_mode,
+            pfp = cf_popensetuid(real_command, arglist, open_mode,
                                  a->contain.owner, a->contain.group,
                                  a->contain.chdir, a->contain.chroot,
                                  a->transaction.background);
-            free(command);
+            free(real_command);
             SeqDestroy(arglist);
         }
 

libpromises/eval_context.c

@@ -2757,6 +2757,18 @@ StringSet *EvalContextVariableTags(const EvalContext *ctx, const VarRef *ref)
     return var_tags;
 }
 
+/**
+ * Whether the variable #ref resolves to is tagged secret.
+ *
+ * Unlike EvalContextVariableGet() with get_secret=false, this answers without
+ * producing a value, so a caller can decide before the plaintext exists.
+ */
+bool EvalContextVariableIsTaggedSecret(const EvalContext *ctx, const VarRef *ref)
+{
+    Variable *var = VariableResolve(ctx, ref);
+    return (var != NULL) && VariableIsSecret(var);
+}
+
 bool EvalContextVariableClearMatch(EvalContext *ctx)
 {
     return VariableTableClear(ctx->match_variables, NULL, NULL, NULL);

libpromises/eval_context.h

@@ -232,6 +232,7 @@ const Promise *EvalContextVariablePromiseGet(const EvalContext *ctx, const VarRe
 bool EvalContextVariableRemoveSpecial(const EvalContext *ctx, SpecialScope scope, const char *lval);
 bool EvalContextVariableRemove(const EvalContext *ctx, const VarRef *ref);
 StringSet *EvalContextVariableTags(const EvalContext *ctx, const VarRef *ref);
+bool EvalContextVariableIsTaggedSecret(const EvalContext *ctx, const VarRef *ref);
 bool EvalContextVariableClearMatch(EvalContext *ctx);
 VariableTableIterator *EvalContextVariableTableIteratorNew(const EvalContext *ctx, const char *ns, const char *scope, const char *lval);
 VariableTableIterator *EvalContextVariableTableFromRefIteratorNew(const EvalContext *ctx, const VarRef *ref);

libpromises/expand.c

@@ -514,6 +514,26 @@ Rval ExpandBundleReference(EvalContext *ctx,
     return RvalNew(NULL, RVAL_TYPE_NOPROMISEE);
 }
 
+/**
+ * What to do with a reference to a secret-tagged variable while expanding.
+ *
+ * DEFER keeps "$(password)" in the string, so nothing between expansion and the
+ * use of the value can leak it; the point of use calls ONLY to substitute it.
+ */
+typedef enum
+{
+    /** Substitute the value, like for any other variable. */
+    EXPAND_SECRETS_INLINE,
+    /** Leave "$(name)" in place, for the point of use to resolve. */
+    EXPAND_SECRETS_DEFER,
+    /** Substitute secrets and only secrets; leave every other reference. */
+    EXPAND_SECRETS_ONLY,
+} ExpandSecretsMode;
+
+static char *ExpandScalarInternal(const EvalContext *ctx, const char *ns,
+                                 const char *scope, const char *string,
+                                 Buffer *out, ExpandSecretsMode secrets);
+
 /**
  * Expand a #string into Buffer #out, returning the pointer to the string
  * itself, inside the Buffer #out. If #out is NULL then the buffer will be
@@ -523,6 +543,43 @@ Rval ExpandBundleReference(EvalContext *ctx,
  */
 char *ExpandScalar(const EvalContext *ctx, const char *ns, const char *scope,
                    const char *string, Buffer *out)
+{
+    return ExpandScalarInternal(ctx, ns, scope, string, out,
+                               EXPAND_SECRETS_INLINE);
+}
+
+/**
+ * As ExpandScalar(), but leaves a secret-tagged variable's reference verbatim.
+ *
+ * The result is not final: the consumer has to call ExpandScalarSecretsOnly()
+ * to get the value in. Anything that only logs the string uses it as it is.
+ */
+char *ExpandScalarKeepSecrets(const EvalContext *ctx, const char *ns,
+                              const char *scope, const char *string,
+                              Buffer *out)
+{
+    return ExpandScalarInternal(ctx, ns, scope, string, out,
+                               EXPAND_SECRETS_DEFER);
+}
+
+/**
+ * Substitute the references ExpandScalarKeepSecrets() left behind, and nothing
+ * else -- a non-secret reference is copied through whether or not it resolves.
+ *
+ * Call it as late as possible, keep the result out of any log, and free it as
+ * soon as the value has been handed over.
+ */
+char *ExpandScalarSecretsOnly(const EvalContext *ctx, const char *ns,
+                              const char *scope, const char *string,
+                              Buffer *out)
+{
+    return ExpandScalarInternal(ctx, ns, scope, string, out,
+                               EXPAND_SECRETS_ONLY);
+}
+
+static char *ExpandScalarInternal(const EvalContext *ctx, const char *ns,
+                                 const char *scope, const char *string,
+                                 Buffer *out, ExpandSecretsMode secrets)
 {
     bool out_belongs_to_us = false;
 
@@ -553,11 +610,16 @@ char *ExpandScalar(const EvalContext *ctx, const char *ns, const char *scope,
         ExtractScalarReference(current_item,  sp, strlen(sp), true);
         sp += BufferSize(current_item) + 2;
 
-        if (IsCf3VarString(BufferData(current_item)))
+        /* ONLY mode re-emits the reference unless it names a secret, and
+         * expanding an inner one first would re-emit a rewritten name. So a
+         * secret used as part of a variable *name* stays unresolved. */
+        if (IsCf3VarString(BufferData(current_item)) &&
+            secrets != EXPAND_SECRETS_ONLY)
         {
             Buffer *temp = BufferCopy(current_item);
             BufferClear(current_item);
-            ExpandScalar(ctx, ns, scope, BufferData(temp), current_item);
+            ExpandScalarInternal(ctx, ns, scope, BufferData(temp), current_item,
+                                secrets);
             BufferDestroy(temp);
         }
 
@@ -566,8 +628,19 @@ char *ExpandScalar(const EvalContext *ctx, const char *ns, const char *scope,
             VarRef *ref = VarRefParseFromNamespaceAndScope(
                 BufferData(current_item),
                 ns, scope, CF_NS, '.');
-            DataType value_type;
-            const void *value = EvalContextVariableGetPlaintext(ctx, ref, &value_type);
+            const bool is_secret = (secrets != EXPAND_SECRETS_INLINE) &&
+                EvalContextVariableIsTaggedSecret(ctx, ref);
+            const bool substitute = (secrets == EXPAND_SECRETS_ONLY)
+                ? is_secret
+                : !is_secret;
+
+            /* Not substituting leaves the type NONE, which falls through to
+             * the re-emit below -- the path an unresolvable variable takes. The
+             * value is never fetched, not even to be discarded. */
+            DataType value_type = CF_DATA_TYPE_NONE;
+            const void *value = substitute
+                ? EvalContextVariableGetPlaintext(ctx, ref, &value_type)
+                : NULL;
             VarRefDestroy(ref);
 
             switch (DataTypeToRvalType(value_type))

libpromises/expand.h

@@ -41,6 +41,10 @@ bool IsExpandable(const char *str);
 
 char *ExpandScalar(const EvalContext *ctx, const char *ns, const char *scope,
                    const char *string, Buffer *out);
+char *ExpandScalarKeepSecrets(const EvalContext *ctx, const char *ns, const char *scope,
+                              const char *string, Buffer *out);
+char *ExpandScalarSecretsOnly(const EvalContext *ctx, const char *ns, const char *scope,
+                              const char *string, Buffer *out);
 Rval ExpandBundleReference(EvalContext *ctx, const char *ns, const char *scope, Rval rval);
 Rval ExpandPrivateRval(const EvalContext *ctx, const char *ns, const char *scope, const void *rval_item, RvalType rval_type);
 Rlist *ExpandList(const EvalContext *ctx, const char *ns, const char *scope, const Rlist *list, int expandnaked);

libpromises/promises.c

@@ -658,6 +658,19 @@ static void DereferenceAndPutComment(Promise* pp, const char *comment)
     }
 }
 
+/**
+ * Whether this promise type fetches secret values itself, at the point of use.
+ *
+ * Its promiser keeps the references, so the lock name, $(this.promiser) and the
+ * actuator's own log lines cannot leak the value. In exchange the actuator has
+ * to resolve them before use; see RepairExec().
+ */
+static bool PromiseTypeResolvesSecretsAtUse(const Promise *pp)
+{
+    assert(pp != NULL);
+    return StringEqual(PromiseGetPromiseType(pp), "commands");
+}
+
 Promise *ExpandDeRefPromise(EvalContext *ctx, const Promise *pp, bool *excluded)
 {
     assert(pp != NULL);
@@ -668,8 +681,18 @@ Promise *ExpandDeRefPromise(EvalContext *ctx, const Promise *pp, bool *excluded)
 
     *excluded = false;
 
-    Rval returnval = ExpandPrivateRval(ctx, PromiseGetNamespace(pp),
-                                       "this", pp->promiser, RVAL_TYPE_SCALAR);
+    Rval returnval;
+    if (PromiseTypeResolvesSecretsAtUse(pp))
+    {
+        returnval = (Rval) { ExpandScalarKeepSecrets(ctx, PromiseGetNamespace(pp),
+                                                     "this", pp->promiser, NULL),
+                             RVAL_TYPE_SCALAR };
+    }
+    else
+    {
+        returnval = ExpandPrivateRval(ctx, PromiseGetNamespace(pp),
+                                      "this", pp->promiser, RVAL_TYPE_SCALAR);
+    }
     if (returnval.item == NULL)
     {
         assert(returnval.type == RVAL_TYPE_LIST ||