Regular expression error when adding a secret

[Yazan Salti]

Hi team,

juju: 3.4.0
Microk8s: v1.27.11
ubuntu: 23.04

In rare cases when adding a secret in a charm, for example using the following code:
```
secret = self.charm.unit.add_secret(
    {"certificate": certificate.certificate},
    label=f"{LIBID}-{certificate.csr}",
    expire=self._get_next_secret_expiry_time(certificate.certificate),
)
```

We get the following error: `ERROR juju.worker.uniter.context cannot apply changes: creating secrets: Regular expression is invalid: nothing to repeat`

I have seen this error 3 times so far, in two cases it has been followed by another error message:
`ERROR juju.worker.uniter.context cannot cleanup secret {"1f8af4a9-aa70-4b87-802f-e0b69d7e95ae" "cnpd36s7imks74rlii70-1"}: cannot access "cnpd36s7imks74rlii70-1"`

I am unable to reproduce the bug, I have been trying to do so by running the following command with different inputs in a loop:
`juju exec --unit tls-certificates-requirer/0 -- secret-add --owner unit --label afd8c2bccf834997afce12c2706d2ede-"$(cat client.csr)" certificate="$(cat client.crt)"`

I'm not sure where is the regex error coming from or if it can be caused by the content of the secret itself, so here is an example secret if creation was successful:

```
juju show-secret cnpbnjvmp25c77krg9l0 --reveal
cnpbnjvmp25c77krg9l0:
  revision: 1
  owner: tls-certificates-requirer/0
  label: |-
    afd8c2bccf834997afce12c2706d2ede------BEGIN CERTIFICATE REQUEST-----
    MIICbjCCAVYCAQAwKTEnMCUGA1UEAwwedGxzLWNlcnRpZmljYXRlcy1ycXVpcmVy
    LTAuYnVnMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAyof4RXm0PSZ6
    OWA1xBEmwHwMDDPu3RxOycKIHCi/Qg6ftp1hOCRbNdgG2e9qNEVNsWukU1KsP1BO
    R5gqD0skc4h7VK0qnzGkzvIDPFBF9nYY6ZlACT7Zio+MFGI3MykIujaKjyfnIkY1
    bKvFi4C/ayQ0fdCi8mMxvllxvX3/sghh3CRcpEJy3OuAlgQM347hzmLFKTLntTh1
    AMf82IxpHlgeCjn8FcZ9e6BArO8s6ad27Viz5STG6CWbOs2XprtBPgc5az31OGsV
    PD/WmMz5pLVsc+i4dcyck/l649Uf8bu7QNSZbGibFEo9hwvc4p0MMfk1aUnpMzV9
    0oe9oLKufQIDAQABoAAwDQYJKoZIhvcNAQELBQADggEBADCmREyDqP+l/5wH6PdW
    TDE84y9evfnh7Oyzwt9sHpUtDwnvciVAPN0rYOXuajrSqtAprVu9TMf/RHwAfNV1
    yoWxTdZthiMJjQ2gJrspruPoY4+/argsfCYRDat2WRUa5AJ7yWirc6vlsGjCY+3D
    8rH3LpoNBdqMKlq+VcEwE4Ss+OQO/uLfhtNepSvu0ud+VaOrtITm6OIY/DxpVYes
    HM6vLPKl21p2InYeJOIMGqqRvt0m9eZhdXwpkxCYSMxSh0NzFIl1KpIQY3ImOYMQ
    sDOKn49LKbQzc0SjzsoUhvupWFATkVsk7FjYWjLCFs5U06nhUyVT80oeeIWVI5C5
    88E=
    -----END CERTIFICATE REQUEST-----
  created: 2024-03-14T08:56:47Z
  updated: 2024-03-14T08:56:47Z
  content:
    certificate: |-
      -----BEGIN CERTIFICATE-----
      MIIC0TCCAbkCFGU6lMNP2B/y/go11Z5idgp98icWMA0GCSqGSIb3DQEBCwUAMCEx
      CzAJBgNVBAYTAlVTMRIwEAYDVQQDDAlwaXp6YS5jb20wHhcNMjQwMzE0MDg1NjQ2
      WhcNMjUwMzE0MDg1NjQ2WjApMScwJQYDVQQDDB50bHMtY2VydGlmaWNhdGVzLXJx
      dWlyZXItMC5idWcwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDKh/hF
      ebQ9Jno5YDXEESbAfAwMM+7dHE7JwogcKL9CDp+2nWE4JFs12AbZ72o0RU2xa6RT
      Uqw/UE5HmCoPSyRziHtUrSqfMaTO8gM8UEX2dhjpmUAJPtmKj4wUYjczKQi6NoqP
      J+ciRjVsq8WLgL9rJDR90KLyYzG+WXG9ff+yCGHcJFykQnLc64CWBAzfjuHOYsUp
      Mue1OHUAx/zYjGkeWB4KOfwVxn17oECs7yzpp3btWLPlJMboJZs6zZemu0E+Bzlr
      PfU4axU8P9aYzPmktWxz6Lh1zJyT+Xrj1R/xu7tA1JlsaJsUSj2HC9zinQwx+TVp
      SekzNX3Sh72gsq59AgMBAAEwDQYJKoZIhvcNAQELBQADggEBAG3QRmq8XgR5/Ws/
      UJcad5vnBswb/VPoqnzSV7ePheF9TJO0l2W2HQ7kR6q7EXtz1TVObuqRGACUW9M0
      ccnFv2uOe3024zs+aohUCA96ZK2LKOrw4a0BIqb+7GYs1EutryvJMPuS+oIQUy2P
      q5R3i9XAk4etYJiS10Y3+ivbkTTbN86kOSO8/ItRgqLb2tKmM5mF/8QO045+zo8u
      P60UyUzvsd2iRcje8PJEw5y2POeeSMFheaeGkiOEraZMZN2l/dfjFB9LsFvtaLiO
      jQ3G/YPvN0Azr53jwfzApwddf3FV313vMRXo8f9wgr6Z5lyWRbyh5uGLDeL6LeLR
      ro/qiR8=
      -----END CERTIFICATE-----
```

[Pedro Guimarães (pguimaraes)]

The label is using the certificate.csr itself in its composition. Now, x509 may have chars that can be considered special (\n, /, +, etc). I guess you fail whenever the CSR contains a char that the label itself cannot have.

Now, I cannot find where the label is passed through a regex. I see two regexes across the `secret-add` path:
1. https://github.com/juju/juju/blob/3.6/core/secrets/createsecret.go#L19
2. Within the agent's uniter, for the URI: https://github.com/juju/juju/blob/3.6/core/secrets/secret.go#L47-L53

Still, I cannot find where the label is checked against a regex. So, this is just a hypothesis rn.

@yazansalti can you point us to the charm that is using the CSR as a label?

In any case, would be easier to hash the CSR and use the hash as a label instead.

[Yazan Salti (yazansalti)]

Hi @pguimaraes,

I had the same hypothesis, I tried to reproduce the issue by running the secret creation in a loop with different CSRs being the label, and I couldn't get the issue in over 1000 iterations. It only comes up within the charm. I tested creating some secrets using the Juju CLI that include some chars I suspected too (chars I know could appear in an x509 certificates), but once again I couldn't reproduce the error.

I looked into the code and couldn't find where secret labels are checked by a regexp either.

This comes from the TLS library and it can happen in any charm that uses the library, here is the line of code in the libaray:
https://github.com/canonical/tls-certificates-interface/blob/main/lib/charms/tls_certificates_interface/v3/tls_certificates.py#L1796

And an example would be vault-k8s, here is an example of the error there
https://github.com/canonical/vault-k8s-operator/actions/runs/8323174224/job/22772297508?pr=209

Are you suggesting to use a different label and see if the issue comes up again?

[John A Meinel (jameinel)]

Juju seems to be validating that the *keys* are valid strings that are alphanumeric, starting with a letter, etc. But we shouldn't be validating the contents of the key.
And the above code:
```
secret = self.charm.unit.add_secret(
    {"certificate": certificate.certificate},
    label=f"{LIBID}-{certificate.csr}",
    expire=self._get_next_secret_expiry_time(certificate.certificate),
)
```

In that code the key is just "label" which is absolutely valid, and AFAICT juju should not be validating the *value*.

The above error string:
ERROR juju.worker.uniter.context cannot apply changes: creating secrets: Regular expression is invalid: nothing to repeat

the first part about 'cannot apply changes' is happening here:
https://github.com/juju/juju/blob/4d6ece0f954a9071a567a7c828e3bb70e3228a94/worker/uniter/runner/context/context.go#L1700

which is an API call back to the controller for it to commit the current Uniter state including all the changes from relation-set/secret-add/etc.
the second part 'creating secrets' would be coming from the API Server:
https://github.com/juju/juju/blob/4d6ece0f954a9071a567a7c828e3bb70e3228a94/apiserver/facades/agent/uniter/uniter.go#L2818

which means the last part about 'Regular expression is invalid: nothing to repeat' would be coming from the call to:
https://github.com/juju/juju/blob/4d6ece0f954a9071a567a7c828e3bb70e3228a94/apiserver/facades/agent/uniter/uniter.go#L2805
"u.SecretsManagerAPI.CreateSecrets"

None of our Regexes should ever include user generated content (we might run a regex over user content, but we should not compile user content *into* a regex that we run).

Googling around, it does appear that "nothing to repeat" will tend to happen if you do something like "\w++" the first + repeats one or more word characters, and the second one has "nothing to repeat".
And it seems plausible that base64 content could very easily end up with a ++ in it (though more frequent would be ==).

But looking at the call to CreateSecrets:
https://github.com/juju/juju/blob/4d6ece0f954a9071a567a7c828e3bb70e3228a94/apiserver/facades/agent/secretsmanager/secrets.go#L242

That does make a call to things like ParseURI, which does include 2 regexes (validUUID and secretURIParse)
https://github.com/juju/juju/blob/4d6ece0f954a9071a567a7c828e3bb70e3228a94/core/secrets/secret.go#L62

I still don't see a code path where we would be running a regex that includes user data.

That said, if you are running on Microk8s (as given in the description), I believe we use K8s to store the content, and we would be using a different SecretsManager (I'm not positive on this, Ian/Kelvin would need to confirm.)
But in that scenario, its plausible that the secret backend itself is doing something different.

looking at ParseURI, all the error returns are wrapped with a different error message except url.Parse(str) which has its error message returned directly with just errors.Trace()

going back up the stack, createSecret has a bare Trace() if the "OwnerTag" was invalid.
Or if there was a problem calling OwnerToken, ParseURI, CreateSecret.

The particular error string "Regular expression is invalid" is also unlikely to be one that is from J...

[John A Meinel (jameinel)]

In summary, it appears that something around CommitHook ends up calling CreateSecret, which ends up doing a Mongo query, and Mongo is returning "Invalid Regex".
I haven't found a smoking gun there, but I do see that $regex is used extensively in 'state/secrets.go':
$ rg '\$regex'

state/secrets.go
622: bson.D{{"$regex", fmt.Sprintf("%s/.*", uri.ID)}}}}).Select(
650: "_id", bson.D{{"$regex", fmt.Sprintf("%s/%s", uri.ID, revisionRegexp)}},
723: bson.D{{"$regex", fmt.Sprintf("%s/.*", uri.ID)}}}}).Select(
738: "_id", bson.D{{"$regex", fmt.Sprintf("%s/.*", uri.ID)}},
747: "_id", bson.D{{"$regex", fmt.Sprintf("%s#.*", uri.ID)}},
904: "$regex": fmt.Sprintf("%s#.*", uri.ID),
1164: q = bson.D{{"_id", bson.D{{"$regex", uri.ID + "/.*"}}}}
1381: count, err := col.Find(bson.M{"_id": bson.M{"$regex": keyPattern}}).Count()
1392: Id: bson.M{"$regex": keyPattern},
1461: q := bson.D{{"_id", bson.D{{"$regex", id}}}}
1579: {{"_id", bson.D{{"$regex", fmt.Sprintf("%s#.*", uri.ID)}}}},
1602: "_id", bson.D{{"$regex", fmt.Sprintf("%s#.*", uri.ID)}},
1615: "_id", bson.D{{"$regex", fmt.Sprintf("%s#.*", uri.ID)}},
1662: q := bson.D{{"consumer-tag", bson.D{{"$regex", match}}}}
1856: q := bson.D{{"_id", bson.D{{"$regex", key}}}}
1884: err := secretConsumerCollection.Find(bson.D{{"_id", bson.D{{"$regex", idSnippet}}}}).All(&docs)
1899: err := secretConsumerCollection.Find(bson.D{{"_id", bson.D{{"$regex", q}}}}).All(&docs)
1958: w.matchQuery = bson.D{{"consumer-tag", bson.D{{"$regex", match}}}}
2376: "_id": bson.M{"$regex": st.docID(uri.ID + "#.*")},

[John A Meinel (jameinel)]

   bson.D{{"$regex", fmt.Sprintf("%s/.*", uri.ID)}}}}).Select(
^ if the uri.ID started with a +

   _, err = secretRevisionsCollection.Writeable().RemoveAll(bson.D{{
    "_id", bson.D{{"$regex", fmt.Sprintf("%s/%s", uri.ID, revisionRegexp)}},
^ same here

 err = secretRevisionsCollection.Find(bson.D{{"_id",
  bson.D{{"$regex", fmt.Sprintf("%s/.*", uri.ID)}}}}).Select(
^ and here

I'm not sure that these *could* be the problem, as URI.secretURIParse uses:
 idSnippet = `[0-9a-z]{20}`
 uuidSnippet = `[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}`

which would say that the ID must be a 20 character alphanumeric. And there is no place in that for a +.

That particular file makes heavy use of $regex to the database, passing in the ID as the start of the _id search.

It *is* clear that the final error is coming from Mongo, but it isn't clear which possible query could have bad content in the request.

[John A Meinel (jameinel)]

hm, maybe I'm wrong on the 'label' portion. Because if that is the label that you are supplying, and we allow querying by label, its plausible that you could end up putting the content of label into a $regex query of some sort to try and find a secret that matches the label that is supplied.

It is pretty odd to have a full "CSR" in your label, vs more of a declared content.

While I can understand that a Lib may not have a specific use case for a secret, but the *charm* should certainly have a clear reason why it needs that secret to exist, and it should have meaning around the label, and not just a semi-arbitrary (the CSR isn't strictly arbitrary, but it certainly doesn't have *meaning* for a human or for the charm.)

[John A Meinel (jameinel)]

However, I wasn't able to break juju by using weird labels (yet):

$ juju exec --unit dummy-sink/0 'secret-add --label "+mylabel" --description "my special secret" another-key=s3cret'
secret://88d8febd-16bb-4374-8b01-cf48efe50575/cpbolvsefdci6ool96a0
$ juju exec --unit dummy-sink/0 'secret-get --label "+mylabel"'
another-key: s3cret

$ juju exec --unit dummy-sink/0 'secret-add --label "content++mylabel" --description "my special secret" another-key=s3cret'
secret://88d8febd-16bb-4374-8b01-cf48efe50575/cpbom8kefdci6ool96ag
$ juju exec --unit dummy-sink/0 'secret-get --label "content++mylabel"'
another-key: s3cret

$ juju exec --unit dummy-sink/0 'secret-add --label "content\n++mylabel" --description "my special secret" another-key=s3cret'
secret://88d8febd-16bb-4374-8b01-cf48efe50575/cpbonfcefdci6ool96b0

I do feel like Juju should probably be vetting the 'label' attribute, and not allowing an arbitrary string.

I don't know that we need to go all the way to "[a-z0-9]+" but we might. Having it allow a multi-line text content that can include arbitrary characters seems to go very *far* away from what we expect a Label to be.

[John A Meinel (jameinel)]

So it is the label attribute that is causing the failure in the DB layer. It took me a lot of hunting to find but I ended up getting:

$ juju exec --unit dummy-sink/0 "secret-add --label 'content-\u263a' --description 'my special secret' another-key=better"
secret://88d8febd-16bb-4374-8b01-cf48efe50575/cpbotlkefdci6ool96c0
creating secrets: Regular expression is invalid: PCRE does not support \L, \l, \N{name}, \U, or \u

I don't know what magic strings in the CSR could cause the "$regex" to fail, and arguably juju should be escaping the query it is supplying.
But we *also* should not be supporting arbitrary strings in our Label parameter.

[John A Meinel (jameinel)]

This is my best guess as to the regex query that ends up failing :
https://github.com/juju/juju/blob/4d6ece0f954a9071a567a7c828e3bb70e3228a94/state/secrets.go#L1361

uniqueSecretLabelBaseOps is trying to make sure that we don't have 2 secrets with the same label, and thus needs to do a form of regex query against the database to see if there is another entry that collides.

If we wanted to support arbitrayr labels, then these lines:
https://github.com/juju/juju/blob/4d6ece0f954a9071a567a7c828e3bb70e3228a94/state/secrets.go#L1339-L1341

and these
https://github.com/juju/juju/blob/4d6ece0f954a9071a567a7c828e3bb70e3228a94/state/secrets.go#L1349-L1351

probably need to escape the 'secretOwnerLabelKeyPrefix' values.

[Ian Booth (wallyworld)]

https://github.com/juju/juju/pull/17446

[John A Meinel (jameinel)]

I spoke with Harry and Ian about this. And it seems when designing "label", they did intend that it could contain "programmatic" data. They knew that you would have to map from the label back to some sort of context, and they wanted to make it a little bit easier (so you could use, eg JSON, to encode the context that you wanted to pull out).

So for now, we're just planning on properly escaping our DB queries.

I do think that putting the CSR content as the label is still a bit weird, but as we are intending to have arbitrary encoding supported, we'll go with escaping our queries against Mongodb.

[Yazan Salti (yazansalti)]

Thank you for looking into this and fixing it. And on our side we took your advice and changed the label not to use the CSR.