Encryption of TXT records not working as per documentation

[theloneexplorerquest]

Description
Fix the issue that Encryption of TXT records does take base64 encryption key.

1. Use base64.URLEncoding instead of base64.StdEncoding: Since TXT records are often used in DNS settings, which can be included in URLs, it makes sense to use URL-safe characters to avoid potential issues with special characters that have different meanings in URLs. Also per document https://github.com/kubernetes-sigs/external-dns/blob/master/docs/registry/txt.md#encryption the key we generated is URL safe.
2. Update function EncryptText and DecryptText to take encode aes key as input however decode before encrypt and decrypt. Matching doc description.
3. update unit test.

Fixes #3992

Checklist

• Unit tests updated
• End user documentation updated

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign johngmyers for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @theloneexplorerquest. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mloiseleur]

Interesting.
🤔 How users are supposed to move from current encryption to this system ?
/ok-to-test

[theloneexplorerquest]

Interesting. 🤔 How users are supposed to move from current encryption to this system ? /ok-to-test

I am not sure haha. Probably need some more investigation.

[mloiseleur]

For the migration path, there are various options:

• Documentation on how to migrate manually
• CLI arg to restore previous behavior
• CLI arg to launch a specific, one-time, migration command

[theloneexplorerquest]

Hi @mloiseleur

Thank you for your suggestion. I'm inclined towards documenting the process for a manual migration.

However, even when proceeding manually, the process appears to be non-trivial. Here's my query:

Is it necessary to always keep at least one encrypted TXT record in the system for the sake of high availability?
I am considering a method where individuals manually delete the TXT record at the provider's end and then the newly recreated record will be created automatically (after they update the key).

[mloiseleur]

If I understand correctly external-dns documentation about this feature, the txt records contains only metadata.

=> But I'm unsure, is this really safe to stop external-dns previous version, remove TXT records and start external-dns new version ? 🤔 Will this really just re-create new encrypted TXT records without any unexpected side effects ?

[szuecs]

This will break users that are already using txt encryption as far as I understand.
Maybe we should provide a migration path, so for example if it fails to decrypt we use the former StdEncoding.

[szuecs]

I think we should have a fallback on the read path to ensure we can read old encrypted txt records, which we will update in the next iteration anyways.
Maybe that's something to validate that we re-encrypt old encrypted txt records if we use a different encoding....

Right now I think we would just break users, which is not acceptable.

[theloneexplorerquest]

@szuecs thanks for suggestion.

Sorry I was a bit busy in last few weeks.

do you think we should:

1. provide additional boolean argument, if the argument is supplied, we use the old encryption protocol.
2. have a try-fail logic in code, if we cannot decrypt with new protocol, attempt to decrypt with old protocol. --> I think this is what you mean by fallback?
   Thanks!

[mloiseleur]

@theloneexplorerquest I know you didn't ask me but both options sound good to me.

[szuecs]

I would go for option 2, because users should not care about configuring some option correctly. I expect that if we re-encrypt then it will not do much harm and does the automatic migration.