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: Couchbase insecure certificate validation #9458

Merged

Conversation

akrantz01
Copy link
Contributor

Required for all PRs:

  • Updated associated README.md.
  • Wrote appropriate unit tests.

resolves #9454

Fixes a bug where the couchbase plugin would fail to retrieve bucket stats due to a self-signed certificate. Prior, only the bucket stats would fail because the couchbase library skips certificate verification by default unless a keypair and optional root certificate are provided.

@akrantz01 akrantz01 added regression something that used to work, but is now broken fix pr to fix corresponding bug area/couchbase labels Jun 30, 2021
@srebhan srebhan self-assigned this Jul 6, 2021
@srebhan
Copy link
Member

srebhan commented Jul 6, 2021

Hey @akrantz01 thanks for looking into this!

TBH, I'm not sure this is the right approach, as it uses an "insecure by default" approach without allowing the user to enforce strict security rules. So I think you should either replicate the client behavior (only insecure if no certificate is provided) OR (and this is my preference) you allow the user to set insecure_skip_verify as an option.

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

would like to see insecure_skip_verify default to false (we don't want to help users be insecure), and I think this doesn't work for connecting to couchbase without TLS.

@sspaink sspaink changed the title Couchbase insecure certificate validation fix: Couchbase insecure certificate validation Aug 5, 2021
@akrantz01 akrantz01 force-pushed the 9454-couchbase-certificate-verification branch from 7f6d502 to 94349b8 Compare August 5, 2021 22:15
@sspaink sspaink added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Aug 24, 2021
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me and also matches @ssoroka 's requests I think.

@srebhan srebhan requested review from ssoroka and reimda September 6, 2021 11:52
@MyaLongmire MyaLongmire added waiting for response waiting for response from contributor and removed ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. labels Sep 16, 2021
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks perfect to me. Thanks @akrantz01 for this nice PR!

Copy link
Contributor

@reimda reimda left a comment

Choose a reason for hiding this comment

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

Seems like the right thing to do to set the library's TLS settings based on ours.

@reimda
Copy link
Contributor

reimda commented Sep 23, 2021

Hey @akrantz01 could you resolve the conflicts and I'll merge this? Thanks again!

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label Sep 23, 2021
@akrantz01 akrantz01 force-pushed the 9454-couchbase-certificate-verification branch from 5a9691e to 63e4479 Compare September 24, 2021 01:47
@srebhan srebhan removed the request for review from ssoroka September 28, 2021 12:10
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Still looks good.

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Sep 28, 2021
@powersj
Copy link
Contributor

powersj commented Sep 29, 2021

I want to click merge, but lint is failing in couchbase_test.go. Tried editing it in GitHub, but it doesn't show as an issue. Pulling down the MP does show the issue, so typing make fmt and pushing a final commit should do it!

@powersj powersj merged commit 872b29b into influxdata:master Sep 29, 2021
@powersj
Copy link
Contributor

powersj commented Sep 29, 2021

@akrantz01 thank you for bringing this one home!

reimda pushed a commit that referenced this pull request Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/couchbase fix pr to fix corresponding bug ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. regression something that used to work, but is now broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Couchbase plugin stop working due to certificate verification of an self-signed certificate
7 participants