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

server, security: Fix one-way connectivity with connect cmd #63589

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

itsbilal
Copy link
Member

@itsbilal itsbilal commented Apr 13, 2021

Informs #60632.

Previously, non-trust-leader nodes couldn't connect back
to the trust leader due to the presence of the wrong
ca-client.crt on their disk; the main CA cert/key was
being written in four places.

This change fixes that bug,
and also creates a new client.node.crt certificate to prevent
other subsequent errors from being thrown.

Fixes #61624.

Release note: None.

@itsbilal itsbilal requested review from knz and aaron-crl April 13, 2021 21:17
@itsbilal itsbilal requested a review from a team as a code owner April 13, 2021 21:17
@itsbilal itsbilal self-assigned this Apr 13, 2021
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@itsbilal
Copy link
Member Author

@knz This is the minimal fix for #61624 that you brought up.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks deceptively simple but the test suite is trying to tell you something now.

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl)

Previously, non-trust-leader nodes couldn't connect back
to the trust leader due to the presence of the wrong
`ca-client.crt` on their disk; the main CA cert/key was
being written in four places.

This change fixes that bug,
and also creates a new `client.node.crt` certificate to prevent
other subsequent errors from being thrown.

Fixes cockroachdb#61624.

Release note: None.
@itsbilal
Copy link
Member Author

Fixed the tests - they were testing for the old setup of things. PTAL.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl)

@knz
Copy link
Contributor

knz commented Apr 19, 2021

I'm going to go ahead and merge this since I'd like to use it in follow-up work.

bors r+

knz added a commit to knz/cockroach that referenced this pull request Apr 19, 2021
…nnect`

The end-to-end test for the new `connect` command was incomplete,
because of issue cockroachdb#61624 that was blocking the functionality.

Now that cockroachdb#63589 is in, we can add the missing test.

Release note: None
@craig
Copy link
Contributor

craig bot commented Apr 19, 2021

Build failed:

@knz
Copy link
Contributor

knz commented Apr 19, 2021

Flake: #63844

@knz
Copy link
Contributor

knz commented Apr 19, 2021

flaking test is fixed

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 19, 2021

Build succeeded:

@craig craig bot merged commit 4dc05cc into cockroachdb:master Apr 19, 2021
knz added a commit to knz/cockroach that referenced this pull request Apr 19, 2021
…nnect`

The end-to-end test for the new `connect` command was incomplete,
because of issue cockroachdb#61624 that was blocking the functionality.

Now that cockroachdb#63589 is in, we can add the missing test.

Release note: None
craig bot pushed a commit that referenced this pull request Apr 20, 2021
63846: cli/interactive_tests: complete the end-to-end test for `cockroach connect` r=itsbilal a=knz

The end-to-end test for the new `connect` command was incomplete,
because of issue #61624 that was blocking the functionality.

Now that #63589 is in, we can add the missing test.

Release note: None

63921: schemaexpr: fix data race in ProcessColumnSet r=adityamaru a=postamar

This commit fixes a data race introduced by my recent changes tracked
under #63755, involving the generalized use of catalog.Column instead of
descpb.ColumnDescriptor.

Fixes #63907

Release note: None

Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Co-authored-by: Marius Posta <marius@cockroachlabs.com>
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.

cli: TLS certs generated via connect cause one-way connectivity problem
3 participants