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

Add SNI TLS Extension when trying to connect to the test object #11

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

EldarEccor
Copy link

Submit a pull request

Thank you for submitting a pull request! To speed up the review process, please ensure that
everything below is true:

  1. This is not a duplicate of an existing pull request.
  2. No existing features have been broken without good reason.
  3. Your commit messages are detailed
  4. The [contribution rules][1] have be carefully read and followed.
  5. Documentation has been updated to reflect your changes.
  6. Tests have been added or updated to reflect your changes.
  7. All tests pass.

Replace any ":question:" below with information about your pull request.

Pull Request Details

Unfortunately, our OpenShift ingress-operator insists on having the SNI extension set in the ClientHello :(

Without or with wrong SNI the ingress-operator itsself with its default certificte is presented, because no route could be properly selected:

[15:39]:~/pkits-distro-assembly-2.4.0 $ openssl s_client -showcerts -connect rproxy-ufs-kkh-tts-zru.apps.tst.ehealth.svc.meshcore.net:443 -servername " "
CONNECTED(000001E4)
---
Certificate chain
 0 s:CN = *.apps.tst.ehealth.svc.meshcore.net
   i:CN = ingress-operator@1690795121

With SNI everybody is happy and I can get to my test object:

[16:43]:~/pkits-distro-assembly-2.4.0 $ openssl s_client -showcerts -connect rproxy-ufs-kkh-tts-zru.apps.tst.ehealth.svc.meshcore.net:443 -servername rproxy-ufs-kkh-tts-zru.apps.tst.ehealth.svc.meshcore.net
CONNECTED(000001F4)
---
no peer certificate available
---
No client certificate CA names sent
---
SSL handshake has read 71 bytes and written 358 bytes
Verification: OK
---
New, (NONE), Cipher is (NONE)
Secure Renegotiation IS supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
SSL-Session:
    Protocol  : TLSv1.2
...
    Extended master secret: yes
---
9668:error:14094417:SSL routines:ssl3_read_bytes:sslv3 alert illegal parameter:../openssl-1.1.1k/ssl/record/rec_layer_s3.c:1543:SSL alert number 47

BouncyCastle will not automatically add SNI when using the TLS Client API.
Added a little something to change that.

Breaking Changes

Describe what features are broken by this pull request and why, if any.

SNI is supposed to be an optional extension, so I hope none?!

Other Relevant Information

If possible please create an "official" release right away, so we can use a proper release to create our test reports :)

@RStaeber
Copy link
Member

Hello EldarEccor,
Thank you for your pull request. We'll probably take a closer look at this next week.
Certainly it can be built in, but it must be enabled via configuration (pkits.yml).
Good thing you already deleted your maven.yml ;-)
Internal Ticket ID is PKITS-603.

@RStaeber
Copy link
Member

in progress now

if (StringUtils.isNotBlank(hostName) && !serverAddress.isLoopbackAddress()){
log.info("Try to connect with S(erver)N(ame)I(ndication): \"{}\"", hostName);
SNIHostName sniHostName = new SNIHostName(hostName);
sslParameters.setServerNames(List.of(sniHostName));
Copy link
Member

Choose a reason for hiding this comment

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

This should be placed before line 138, shouldn't it?

@RStaeber
Copy link
Member

will be part of the release next week

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.

2 participants