Skip to content

GnuTLS: load and send X.509 certificate chains#458

Closed
yadij wants to merge 0 commit into
squid-cache:masterfrom
yadij:v5-gnutls-chainload
Closed

GnuTLS: load and send X.509 certificate chains#458
yadij wants to merge 0 commit into
squid-cache:masterfrom
yadij:v5-gnutls-chainload

Conversation

@yadij

@yadij yadij commented Aug 14, 2019 •
edited by rousskov
Loading

Copy link
Copy Markdown
Contributor

Feature parity with OpenSSL in terms of loading and validating a chain
of X.509 intermediary certificates from a PEM file during configuration,
and delivery of the resulting chain on TLS ServerHello handshakes.

@squid-anubis squid-anubis added M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels labels Aug 14, 2019
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Aug 21, 2019
@yadij

yadij commented Aug 22, 2019

Copy link
Copy Markdown
Contributor Author

If there are no objections I will mark this as cleared for merge in about 2 days.

rousskov
rousskov requested changes Aug 22, 2019
View reviewed changes

@rousskov rousskov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The biggest change request I recall is the use of exceptions instead of (currently broken) error codes, but I believe all of the suggested changes can be implemented without serious rewrites.

Were the OpenSSL changes tested?

Comment thread src/security/CertGadgets.h Outdated
Comment thread src/security/KeyData.cc Outdated
Comment thread src/security/KeyData.cc Outdated
Comment thread src/security/KeyData.cc Outdated
Comment thread src/security/CertGadgets.h Outdated
Comment thread src/security/ServerOptions.cc Outdated
Comment thread src/security/ServerOptions.cc Outdated
Comment thread src/security/forward.h Outdated
Comment thread src/security/forward.h Outdated
Comment thread src/security/CertGadgets.cc Outdated

#else
// fail
out.append("[not implemented]");

@rousskov rousskov Aug 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Feels strange that we display WARNINGs when something goes wrong while extracting the certificate subject but appear to be happy when we cannot even start extracting. If this code is reached, should not this be an even bigger WARNING (or equivalent)? If this is intentional, we should document the reason behind this surprising difference.

@yadij yadij force-pushed the v5-gnutls-chainload branch from 0240e42 to cf1f20c Compare August 26, 2019 09:28
@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Sep 10, 2019
@yadij yadij force-pushed the v5-gnutls-chainload branch from 745ecff to a05329a Compare December 1, 2019 14:10
@yadij yadij added the Squid-5 label Dec 23, 2019
@ilatypov

ilatypov commented May 21, 2020 •
edited
Loading

Copy link
Copy Markdown

It took a few hours to find this pull request and understand that Debian's squid serving CONNECT requests on the https_port will not be accepted by browsers because the squid server certificate has an intermediate signer. Oh well, back to unencrypted CONNECTs exposing my basic authentication, then.

@rousskov

rousskov commented May 21, 2020

Copy link
Copy Markdown
Contributor

@ilatypov, please note that instead of exposing your basic authentication you can build Squid with OpenSSL OR use stunnel-like wrapper in front of an http_port to encrypt CONNECT on the wire.

@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels May 23, 2020
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Jun 2, 2020
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Aug 5, 2020
@yadij yadij force-pushed the v5-gnutls-chainload branch 2 times, most recently from 06a7ce0 to 6b949fc Compare May 14, 2021 12:30
@yadij yadij added the review-1 label May 22, 2021
@yadij yadij closed this May 25, 2021
@yadij yadij force-pushed the v5-gnutls-chainload branch from b9afe56 to e227da8 Compare May 25, 2021 10:52
@yadij yadij deleted the v5-gnutls-chainload branch May 25, 2021 12:43
@yadij yadij mentioned this pull request May 25, 2021
Open
@yadij

yadij commented May 25, 2021

Copy link
Copy Markdown
Contributor Author

Screwed up a rebase for this PR. GIthub automatically closed and will not let me re-open it. So generated a clean replacement as PR #831

@rousskov rousskov removed S-waiting-for-author author action is expected (and usually required) backport-to-v5 labels Oct 23, 2021
@rousskov rousskov removed the review-1 label Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rousskov rousskov rousskov requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yadij @ilatypov @rousskov @squid-anubis