Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: etcd peer sans list #5806

Closed

Conversation

sergelogvinov
Copy link
Sponsor Contributor

@sergelogvinov sergelogvinov commented Jun 25, 2022

Before generate the peer certificate, we will check IP existence on the node.

Why? (reasoning)

The node does not have all IP addresses when we create Etcd certificate. In this case, peer cert does not have an advertised IP.

Acceptance

Please use the following checklist:

  • you linked an issue (if applicable)
  • you included tests (if applicable)
  • you ran conformance (make conformance)
  • you formatted your code (make fmt)
  • you linted your code (make lint)
  • you generated documentation (make docs)
  • you ran unit-tests (make unit-tests)

See make help for a description of the available targets.


This change is Reviewable

Before generate the peer certificate, we will check IP existence on the node.

Signed-off-by: Serge Logvinov <serge.logvinov@sinextra.dev>
@frezbo
Copy link
Member

frezbo commented Jun 26, 2022

isn't this already fixed by #5135 ? 🤔

@smira
Copy link
Member

smira commented Jun 27, 2022

it's not clear to me what the goal is here... to include less IPs?

@sergelogvinov
Copy link
Sponsor Contributor Author

Hello, thank for your reply.

Unfortunately, isn't work for me. I have two interfaces (public/private). The second one appears a little late (more than 1 second). I use etcd.subnet to check IP existence on the node. If not - observe the node IPs again.
In dmesg logs, I sow that Talos tried to create a cert 3 times after this changes (no address matched the provided subnet).

And I've removed the DNS record from peer certs because Talos uses only IPs to create member list (p2p communication). Etcd won't check reverse DNS lookup (https://etcd.io/docs/v3.3/op-guide/security/#notes-for-tls-authentication etcd-io/etcd#7767). And any other IP from the certificate which not used at all. Peer SAN needs only one IP - advertised IP.

Plus - an unauthorized client wouldn't know all node IPs.

The server etcd certificate (for client connections) more complicated - I kept it as is.

@smira
Copy link
Member

smira commented Jun 27, 2022

I understand the issue, but I feel the fix should be more complicated.

As I see it, peer certificate should only contain IPs from the etcd subnet?

@sergelogvinov
Copy link
Sponsor Contributor Author

I understand the issue, but I feel the fix should be more complicated.

As I see it, peer certificate should only contain IPs from the etcd subnet?

Yep, only one IP - advertised ip, and sometimes i think about 127.0.0.1/::1 should be removed too...

smira added a commit to smira/talos that referenced this pull request Jul 8, 2022
Remove code which is no longer needed.

Taken from siderolabs#5806

Co-authored-by: Serge Logvinov <serge.logvinov@sinextra.dev>
Signed-off-by: Andrey Smirnov <andrey.smirnov@talos-systems.com>
smira added a commit to smira/talos that referenced this pull request Jul 8, 2022
Remove code which is no longer needed.

Taken from siderolabs#5806

Co-authored-by: Serge Logvinov <serge.logvinov@sinextra.dev>
Signed-off-by: Andrey Smirnov <andrey.smirnov@talos-systems.com>
smira added a commit to smira/talos that referenced this pull request Jul 13, 2022
Remove code which is no longer needed.

Taken from siderolabs#5806

Co-authored-by: Serge Logvinov <serge.logvinov@sinextra.dev>
Signed-off-by: Andrey Smirnov <andrey.smirnov@talos-systems.com>
(cherry picked from commit 1361225)
@smira
Copy link
Member

smira commented Aug 2, 2022

I hope with #6012 merge in, the etcd issues should be resolved in a bit different way.

  • etcd certs still include all addresses, and they are updated on the fly as etcd is able to reload them.
  • Talos waits for the configured etcd subnet to be available as an address on the node before starting etcd.

@smira
Copy link
Member

smira commented Aug 8, 2022

@sergelogvinov please let us know if you see any improvement with master, we should also cut next alpha today or tomorrow.

@sergelogvinov
Copy link
Sponsor Contributor Author

Thank you.

Peer cert has my second IP address now.

# talosctl --talosconfig _cfgs/talosconfig --nodes 172.16.0.11 version                                                           
Client:
        Tag:         v1.1.0
        SHA:         d55a1871
        Built:       
        Go version:  go1.18.3
        OS/Arch:     darwin/arm64
Server:
        NODE:        172.16.0.11
        Tag:         v1.2.0-alpha.2
        SHA:         be351dcb
        Built:       
        Go version:  go1.19
        OS/Arch:     linux/amd6

# openssl s_client -showcerts -connect XXX.XXX.XXX.XXX:2380 |  openssl x509 -tex | grep 172.16.0.11
# echo $?                                                                                        
0

@smira
Copy link
Member

smira commented Aug 11, 2022

awesome, more fixes are coming for listen address as well, and advertised subnets, it should be even better soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants