Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

[io.net] Add support for acting based on the hostname/port as supplied by sslEngine #6651

Merged
merged 1 commit into from
Jan 1, 2019
Merged

[io.net] Add support for acting based on the hostname/port as supplied by sslEngine #6651

merged 1 commit into from
Jan 1, 2019

Conversation

martinvw
Copy link
Contributor

@martinvw martinvw commented Dec 8, 2018

Also-by: Matthew Bowman github@otr.mx
Signed-off-by: Martin van Wingerden martin@martinvw.nl

@openhab-bot
Copy link
Contributor

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/ubiquiti-unifi-binding-feature-discussion/14520/677

@mgbowman
Copy link

mgbowman commented Dec 9, 2018

With these changes, we now have TlsProvider.getHostName() potentially returning two different values: the CN (common name) from the certificate or host:port from the actual connection.

Maybe we should expand the TlsProvider interface with more specific calls based on the TLS attributes that are actually getting checked:

TlsProvider.getCommonName()
TlsProvider.getPeerHost()
TlsProvider.getPeerPort()

This way depending on which checkServerTrusted(...) method is called in ExtensibleTrustManager, the proper format can be constructed internally.

Copy link

@mgbowman mgbowman left a comment

Choose a reason for hiding this comment

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

Just some ideas...

Copy link

@mgbowman mgbowman left a comment

Choose a reason for hiding this comment

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

Possible NPE?

@martinvw
Copy link
Contributor Author

martinvw commented Dec 9, 2018

Note to self the tests have not yet been adapted, to the changed class name.

@martinvw
Copy link
Contributor Author

martinvw commented Dec 9, 2018

With these changes, we now have TlsProvider.getHostName() potentially returning two different values: the CN (common name) from the certificate or host:port from the actual connection.

It is a little more complex, because it could also be matched against the subject-alternative-name, but conceptionally they are all hostnames so that should not matter too much.

Would it address your concern it we just map them on hostname (being peer, common-name, subject-alternative-name or whatever) and have port as a separate method defaulting to 443 but only being used when checking based on peer.

This way depending on which checkServerTrusted(...) method is called in ExtensibleTrustManager, the proper format can be constructed internally.

I do now fallback to both, which is imho a proper (backwards compatible) implementation

@kaikreuzer
Copy link
Contributor

Seems this needs to be rebased.

@martinvw
Copy link
Contributor Author

martinvw commented Dec 9, 2018

Indeed, but we also have decisions to be made and the build is broken 😊

Hope to work on it tonight, should I mark it WIP? Do you have an opinion about splitting up the host related methods?

@martinvw
Copy link
Contributor Author

martinvw commented Dec 9, 2018

I updated the PR, what remains is a decision about the host / common name / port methods.

@kaikreuzer
Copy link
Contributor

I don't have any opinion as I didn't look into the details of this PR so far. As it isn't relevant for the openHAB 2.4 release (correct?), I don't have it at the top of my prio list right now...

@martinvw
Copy link
Contributor Author

martinvw commented Dec 9, 2018

As it isn't relevant for the openHAB 2.4 release (correct?), I don't have it at the top of my prio list right now...

Correct and thus 👍

…d by sslEngine

Also expose the `ExtensibleTrustManager` to allow dynamic injection.
Added remark about immutable implementations

Also-by: Matthew Bowman <github@otr.mx>
Signed-off-by: Martin van Wingerden <martin@martinvw.nl>
Copy link
Contributor

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@kaikreuzer kaikreuzer merged commit dbb59b6 into eclipse-archived:master Jan 1, 2019
@martinvw martinvw deleted the feature/io-net-use-ssl-engine branch January 2, 2019 18:24
@openhab-bot
Copy link
Contributor

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/ubiquiti-unifi-binding-feature-discussion/14520/834

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

Successfully merging this pull request may close these issues.

4 participants