Try #3: Support encrypted DNS txt records #1828

vsychov · 2020-10-20T10:03:35Z

Hi guys,

I found few pull request about txt records encryption (#1314, #1115, #1538), that have conflicts now, or not merged.
This PR have latest PR (#1538) with resolved conflicts, hope it will help you to review and merge it.

Fixes #1825
Fixes #854

Checklist

Unit tests updated
End user documentation updated

vsychov · 2020-10-20T10:04:57Z

/assign @njuettner

vsychov · 2020-10-20T12:32:11Z

/kind feature

Raffo

Thank you for the PR @vsychov . I did a first pass at reviewing, but error handling and a few other things are a bit off and far from a good enough quality. I would suggest, if you are open to that, that you start refactoring the code following the guidelines that I gave.
In a nutshell:

error handling according to go guidelines (no valid values when error, don't hide errors, etc.)
comments that are useful to generate godocs
use American English for all code.

Once a refactor is done I will proceed with a code review.

endpoint/crypto.go

vsychov · 2020-10-22T14:55:44Z

@Raffo, I made some code refactoring, fixed error handling, and crypto functions logic little bit, do you have any other comments?

vsychov · 2020-11-06T11:42:54Z

Hi @Raffo , is all good, or something more changes needed for merge it?

marcgascon · 2020-12-14T08:55:41Z

Anything missing in this PR beside conflicts? I am really interested in this new feature. 😄

vsychov · 2021-03-02T20:34:16Z

I fixed conflicts with master, do It need more changes for be merged, @Raffo?

Raffo

@vsychov There are still many changes to be done, please check again that all errors are handled, comments are correct and that we do not mask possible errors. Generally speaking we also don't want to log and return err.

Raffo · 2021-03-03T08:53:00Z

endpoint/crypto.go

+
+// EncryptText gzip input data and encrypts it using the supplied AES key
+func EncryptText(text string, aesKey []byte) (string, error) {
+	block, _ := aes.NewCipher(aesKey)


There is a unhandled error here, please handle it.

Raffo · 2021-03-03T08:53:21Z

endpoint/crypto.go

+	data := []byte(text)
+	data, err = compressData(data)
+	if err != nil {
+		log.Debugf("Failed to compress data based on the text %#v. Got error %#v.", text, err)


We don't want to log and return the error. If you need to add a message, please wrap the error.

Raffo · 2021-03-03T08:53:45Z

endpoint/crypto.go

+	data := []byte(text)
+	data, err = compressData(data)


Suggested change

data := []byte(text)

data, err = compressData(data)

data, err = compressData([]byte(text))

Raffo · 2021-03-03T08:54:28Z

endpoint/crypto.go

+	return string(plaindata), nil
+}
+
+// Decompress gzip compressed data


code comment is wrong

Suggested change

// Decompress gzip compressed data

// decompressData gzip compressed data

Raffo · 2021-03-03T08:54:40Z

endpoint/crypto.go

+	return b.Bytes(), nil
+}
+
+// Compress data by gzip, for minify data stored in registry


Suggested change

// Compress data by gzip, for minify data stored in registry

// compressData data by gzip, for minify data stored in registry

Raffo · 2021-03-03T08:55:25Z

endpoint/labels.go

+	}
+	return NewLabelsFromStringPlain(labelText)
+}
+
 // Serialize transforms endpoints labels into a external-dns recognizable format string


Suggested change

// Serialize transforms endpoints labels into a external-dns recognizable format string

// SerializePlain transforms endpoints labels into a external-dns recognizable format string

endpoint/labels.go

Raffo · 2021-03-03T08:56:09Z

endpoint/labels.go

+	if err != nil {
+		log.Debugf("Failed to encrypt the text %#v using the encryption key %#v. Got error %#v.", text, aesKey, err)
+	}


This error is masked. This function should probably return an error.

@Raffo there is a little to do with this mr
@vsychov do you have working fork with txt encryption feature?

@shcherbak, yes, I'm using this feature in a production with the Cloudflare provider since the moment of the PR creation. If you want I can publish a working Docker image for your convenience.

@vsychov this is amazing, thank you
but i think will wait for official chart and image
nicely done, respect

vsychov · 2021-03-03T21:59:20Z

@Raffo, fixed

Mattie112 · 2021-06-03T10:05:03Z

@Raffo Sorry for tagging you but it seems you need to approve this change. Is there any way you can look into this? We would like to use the txt-registry but don't want to expose any more information to the outside world.

SCLogo · 2021-07-08T10:14:15Z

Any reason why this PR not merged yet? It is a very good new feature to not leak any data about your cluster

SCLogo · 2021-07-13T10:57:05Z

@Raffo @njuettner Can we help in anything to merge this PR ? It fixes a huge security leak I think. Thanks

SCLogo · 2021-07-27T10:28:53Z

@vsychov could you please change request change to review request? maybe this is the only issue here.

k8s-triage-robot · 2021-10-25T10:38:49Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

Mattie112 · 2021-10-25T10:59:46Z

We still very much want this feature and it is already a PR, so please /remove-lifecycle stale ?

vsychov · 2021-10-25T14:27:32Z

/remove-lifecycle stale

szuecs · 2023-04-26T07:58:30Z

Please don't merge remote master but fetch master and rebase your branch on top of master.
I saw this in my email and inbox maybe you already fixed it.
Thanks

Signed-off-by: Viacheslav Sychov <viacheslav.sychov@gmail.com>

vsychov · 2023-04-28T18:33:13Z

Please don't merge remote master but fetch master and rebase your branch on top of master. I saw this in my email and inbox maybe you already fixed it. Thanks

@szuecs, I combined all commits into one and then rebased it onto the master branch. Previously, this was nearly impossible due to numerous conflicts in each commit.

Hope now it's ok to merge?

szuecs · 2023-05-08T10:29:40Z

@vsychov I wait for @Raffo to review. From my side lgtm

/lgtm

shcherbak · 2023-05-09T09:31:09Z

is there any reason why this PR not merged yet? @Raffo

Raffo · 2023-05-09T10:31:37Z

Sorry I am really on life support with time. I can't review this. @szuecs please feel free to merge this if you think it's good.

szuecs · 2023-05-09T17:46:43Z

/approve

k8s-ci-robot · 2023-05-09T17:47:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: szuecs, vsychov

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [szuecs]

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

szuecs · 2023-05-09T17:50:49Z

@vsychov thanks for your work and answers. Wow we just merged a PR from 2020, great to see that such old PRs are finally merged and improving the tool for everyone.

shcherbak · 2023-05-09T19:12:13Z

@Raffo please take care of yourself

shcherbak · 2023-05-10T05:37:35Z

@szuecs nicely done! It is time for pushing new tag, isn't it? :) Or talking seriously, when new release is planned?

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 20, 2020

k8s-ci-robot requested review from njuettner and Raffo October 20, 2020 10:03

k8s-ci-robot assigned njuettner Oct 20, 2020

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 20, 2020

Raffo requested changes Oct 21, 2020

View reviewed changes

vsychov requested a review from Raffo October 22, 2020 15:00

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2020

njuettner assigned Raffo and unassigned njuettner Mar 3, 2021

Raffo requested changes Mar 3, 2021

View reviewed changes

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2021

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 25, 2021

vsychov force-pushed the txt-encryption branch from e5cd36d to 735cb75 Compare April 25, 2023 21:49

vsychov closed this Apr 28, 2023

vsychov force-pushed the txt-encryption branch from 6162c3c to eaabf71 Compare April 28, 2023 18:22

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 28, 2023

vsychov added a commit to vsychov/external-dns that referenced this pull request Apr 28, 2023

kubernetes-sigs#1828: Support encrypted DNS txt records

1b22c95

Signed-off-by: Viacheslav Sychov <viacheslav.sychov@gmail.com>

vsychov mentioned this pull request Apr 28, 2023

Deleted #3578

Closed

vsychov reopened this Apr 28, 2023

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 28, 2023

kubernetes-sigs#1828: Support encrypted DNS txt records

2554f9f

Signed-off-by: Viacheslav Sychov <viacheslav.sychov@gmail.com>

vsychov force-pushed the txt-encryption branch from 1b22c95 to 2554f9f Compare April 28, 2023 18:30

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2023

k8s-ci-robot assigned szuecs May 8, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 8, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 9, 2023

k8s-ci-robot merged commit f56e2f6 into kubernetes-sigs:master May 9, 2023

lnhrdt mentioned this pull request Oct 30, 2024

Encryption of TXT records not working as per documentation #3992

Closed

	data := []byte(text)
	data, err = compressData(data)
	data, err = compressData([]byte(text))

	// Decompress gzip compressed data
	// decompressData gzip compressed data

	// Compress data by gzip, for minify data stored in registry
	// compressData data by gzip, for minify data stored in registry

	// Serialize transforms endpoints labels into a external-dns recognizable format string
	// SerializePlain transforms endpoints labels into a external-dns recognizable format string

Try #3: Support encrypted DNS txt records #1828

Try #3: Support encrypted DNS txt records #1828

Conversation

vsychov commented Oct 20, 2020

vsychov commented Oct 20, 2020

vsychov commented Oct 20, 2020

Raffo left a comment

Choose a reason for hiding this comment

vsychov commented Oct 22, 2020

vsychov commented Nov 6, 2020

marcgascon commented Dec 14, 2020

vsychov commented Mar 2, 2021

Raffo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsychov commented Mar 3, 2021

Mattie112 commented Jun 3, 2021

SCLogo commented Jul 8, 2021

SCLogo commented Jul 13, 2021

SCLogo commented Jul 27, 2021

k8s-triage-robot commented Oct 25, 2021

Mattie112 commented Oct 25, 2021

vsychov commented Oct 25, 2021

szuecs commented Apr 26, 2023 • edited Loading

vsychov commented Apr 28, 2023 • edited Loading

szuecs commented May 8, 2023

shcherbak commented May 9, 2023 • edited Loading

Raffo commented May 9, 2023

szuecs commented May 9, 2023

k8s-ci-robot commented May 9, 2023

szuecs commented May 9, 2023

shcherbak commented May 9, 2023

shcherbak commented May 10, 2023 • edited Loading

szuecs commented Apr 26, 2023 •

edited

Loading

vsychov commented Apr 28, 2023 •

edited

Loading

shcherbak commented May 9, 2023 •

edited

Loading

shcherbak commented May 10, 2023 •

edited

Loading