secrets.cipher and secrets.cookie have to be string in the configuration

[mangalaman93]

Preflight checklist

• [X] I could not find a solution in the existing issues, docs, nor discussions.
• [X] I agree to follow this project's Code of Conduct.
• [X] I have read and am following this repository's Contribution Guidelines.
• [ ] This issue affects my Ory Cloud project.
• [X] I have joined the Ory Community Slack.
• [X] I am signed up to the Ory Security Patch Newsletter.

Describe the bug

secrets.cipher and secrets.cookie have to be string in the configuration instead of a full range of 256 char/byte values. It reduces that randomness of the key. Kratos should accept a hex/base64 encoded key as well.

Reproducing the bug

NA

Relevant log output

No response

Relevant configuration

No response

Version

latest

On which operating system are you observing this issue?

No response

In which environment are you deploying?

No response

Additional Context

No response

[aeneasr]

Thank you for the idea. I don't think I understand your question. You can very easily use a PNRG and base64 or hex encoded and then feed it to Ory Kratos. JSON, YAML, and environment variables do not support binary formats and getting a string format for your secret is trivial using e.g. openssh or even /dev/random?

[mangalaman93]

If I run openssl rand 128, it gives me a 32 byte length random key and prints a weird looking sequence. How do I take this sequence and provide it to kratos today?

If you like the idea of allowing a hex encoded key, I am happy to raise a PR.

[aeneasr]

For example with:

openssl rand -base64 32
/dev/urandom tr -dc _A-Z-a-z-0-9 | head -c6

We currently do not accept changes to the secrets pipeline, it is proven and works well. The risk of adding serious regressions with different encoding types is too high. Thank you for the offer though!

[zepatrik]

You could also do

$ openssl rand 128 | base64
wRjeAJ5l6ZVMWEKNANqF0EgAH/q9fWZoB2CwauPUFvsh0yjAfK0RnJV8n4oY56vY5DQZ59iyPbP
GCXzdrqPJEfuY1Ks4jwlI9dsfqF05JKaC0CMWyYgyCq+m5yLoYiv7hGN6Op4QLKyqEEjX8uYQs5iYX5
RY9MoR1qX7BT15A1wd4=

which gives you a nice string to put in your config file.

[aeneasr]

Ah, I understand you point though now, the problem is that some of the keys require a fixed key length such as 32 chars. However, we currently only support non-binary payloads there, so you effectively get a lower entropy because even if you provide hex values it would actually be less than the 256 byte entropy

[aeneasr]

Open to idaes now :)

[mangalaman93]

In the configuration, we could check whether the length of the key is 64 chars or 32 chars. We try to parse it as hex data if it is 64 chars otherwise as binary data if it is 32 chars. We completely ignore the keys that are of any other length as we already do.

[mangalaman93]

for base64, a 32 byte data would become 44 chars. We can support base64 as well if you like. For me, as a user, I am fine with just either base64 or hex. I don't care much as long as I can specify my key accurately.

[mangalaman93]

Though, that only works for secrets.cipher and not for for secrets.cookie.

[zepatrik]

Question is how would you safely migrate? Existing secrets cannot break, so it is hard to determine e.g. by a fixed prefix, as someone might already have that. Maybe add a new key that is secrets.cipher_base64 and deprecate the existing key?

[aeneasr]

I find base64 encoding always difficult because there are four ways of base64 encoding

1. url
2. url without padding
3. default
4. default without padding

so I suggest hex encoding. I think that a hex prefix could do the job here:

secrets:
  session:
  - hex://haf76z237
  - old-secret

WDYT?

[mangalaman93]

I have never seen syntax like hex:// used by a project before. I think the key should always be hex encoded. The project shouldn't allow an ASCII key at all because users may not be careful in using a secret enough key. The way to go forward should be to use another configuration parameter and deprecate the existing one.

[aeneasr]

Sorry, but I disagree here. hex has a limited charset and will break any non-hex string. Changing secret handling in a breaking fashion is extremely dangerous and can cause data corruption. Using a prefix such as base64:// is very common, and a good path forward for this issue. I also think that we should use base64 encoding here and not hex encoding.

[mangalaman93]

What do you mean by "hex has limited charset and will break any non hex string"? Any binary string can be encoded into hex string.

[aeneasr]

The problem is that, if you have a secret like

secrets:
- mylittlesecret

it can not be interpreted as a hex string. That is ok, because we actually want users to use hex strings in the future. However, if your secret is

secrets:
- aabbccdd

it will be interpreted as a hex string after this change. However, the secret is now a much different one than the plaintext secret it was before. This will cause data corruption in cases where users do not apply the upgrade carefully. It's thus not worth the risk to implement this in such a breaking change.

A better solution would be to use base64 encoding which is an alternate way to specify secrets with arbitrary binary data using the URI scheme:

secrets:
- i-am-a-plaintext-secret
- base64://ey....

that way, definind the encoding is explicit and not implicit. If you want to move to a binary secret, appending base64:// is an easy way to do it without introducing potentially devastating effects on upgrading users!

Hope this makes sense :)

[mangalaman93]

This makes sense, thanks for the explanation. The only concern I have with this is that we should completely disallow ASCII secret keys. Users will end up using weak keys and that is not good. We should also provide docs for generating the keys. Additionally, if it were me, I would put explicit instructions for upgrade, and ensure that no corruption happens and use a fixed encoding either hex or base64, given that 1.0 is not out yet.

[aeneasr]

Thank you for the ideas! I generally agree with you. Unfortunately we need to keep BC for now, but can move it to a deprecated key at some point or warn when users use ASCII representation :)

[github-actions[bot]]

Hello contributors!

I am marking this issue as stale as it has not received any engagement from the community or maintainers for a year. That does not imply that the issue has no merit! If you feel strongly about this issue

• open a PR referencing and resolving the issue;
• leave a comment on it and discuss ideas on how you could contribute towards resolving it;
• leave a comment and describe in detail why this issue is critical for your use case;
• open a new issue with updated details and a plan for resolving the issue.

Throughout its lifetime, Ory has received over 10.000 issues and PRs. To sustain that growth, we need to prioritize and focus on issues that are important to the community. A good indication of importance, and thus priority, is activity on a topic.

Unfortunately, burnout has become a topic of concern amongst open-source projects.

It can lead to severe personal and health issues as well as opening catastrophic attack vectors.

The motivation for this automation is to help prioritize issues in the backlog and not ignore, reject, or belittle anyone.

If this issue was marked as stale erroneously you can exempt it by adding the backlog label, assigning someone, or setting a milestone for it.

Thank you for your understanding and to anyone who participated in the conversation! And as written above, please do participate in the conversation if this topic is important to you!

Thank you 🙏✌️