Added ExtensibleTrustManager to be used for the (common) http/websocket client #6281
Conversation
|
@martinvw Did you see the compilation errors on Travis? |
| @@ -66,32 +63,6 @@ public void testGetClients() throws Exception { | |||
| webClientFactory.deactivate(); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testGetHttpClientWithEndpoint() throws Exception { | |||
There was a problem hiding this comment.
as long as we keep the method in place (deprecated), we should imho also leave the tests in place.
There was a problem hiding this comment.
I would like to make it behave as if no endpoint was given, so to keep a test for that behavior in place seems odd to me. Would you prefer to also keep the behavior?
I’m planning to add new tests for the new method before marking it complete.
There was a problem hiding this comment.
Would you prefer to also keep the behavior?
YES! That's what backward compatibility is about :-)
| import org.eclipse.jdt.annotation.NonNullByDefault; | ||
|
|
||
| /** | ||
| * Provides a TrustManager for supported endpoints |
There was a problem hiding this comment.
code doesn't look as if it does
There was a problem hiding this comment.
Yes indeed will most likely update code and comment 👍
| * @author Martin van Wingerden - Initial Contribution | ||
| */ | ||
| @Component(immediate = true) | ||
| public class ESHTrustManager extends X509ExtendedTrustManager { |
There was a problem hiding this comment.
We actually try to avoid the TLA "ESH" in the code - maybe better call it "SmartHomeTrustManager" or maybe even better "ExtensibleTrustManager"?
| logger.debug("http client for endpoint {} requested", endpoint); | ||
| @Deprecated | ||
| public HttpClient createHttpClient(@NonNull String consumerName, @NonNull String endpoint) { | ||
| return createHttpClient(consumerName); |
There was a problem hiding this comment.
Err, by deprecating the feature, the idea was to keep it working just as before - i.e. keep using the (also deprecated) TrustManagerProvider. Otherwise, this would break the backward compatibility and if we break it, there wouldn't be much sense to keep the method as deprecated in place.
| * @author Martin van Wingerden - Initial Contribution | ||
| */ | ||
| @NonNullByDefault | ||
| public interface HostnameKeyStoreMapping { |
There was a problem hiding this comment.
For being the major public extension point (interface) that add-on developers are expected to implement, this is imho a very cryptic name and also does not indicate at all that this is expected to be a service to be registered. How about something like TLSCertificateProvider or similar?
| * | ||
| * @return | ||
| */ | ||
| KeyStore getKeyStore(); |
There was a problem hiding this comment.
Why not let the provider directly provide certificate resources and have them wrapped into KeyStores by the framework? I am pretty sure that many people would not know, how to provide a KeyStore here.
There was a problem hiding this comment.
This is a point where we were still in doubt in the related issue, would it be enough to only have an option to provide a certificate. Or should there also be an option to provide a TrustManager so that you can really tweak everything?
I do indeed agree that the abstraction of a keystore is not really useful it was mainly chosen for technical reasons.
Note however that to accept all certificates for a single host or similar requirements something like providing a TrustManager is needed..
Or we could make that an option of the HttpUtil because a lot of self hosted services run on local host(s) so working on the common name / SAN could be a problem anyway.
There was a problem hiding this comment.
Hm, couldn't there different interfaces be supported?
For example (which one needs to be discusses):
HostnameCeritifcateMappingHostnameKeyStoreMappingHostnameTrustStoreMapping
If a lookup is done, we could define that the most customizable wins. First check if there is a truststore, then if there is a keystore, then if there is a certificate mapping...
There was a problem hiding this comment.
Nice idea!
Only mildly complicating thing is that we have both the common name and the subject alternative names, so it might become an intensive lookup for every request. Because some certificates have 100 SANs so that would be 300 lookups if none is available. But definitely worth discussing and exploring this.
There was a problem hiding this comment.
They would still register for specific endpoints and in practice, I would think that there'll always only be one service registered per domain name.
I agree that having different interfaces for different complexities make sense - and for the start, I think the certificate provider is all we need for now (anything else can be added later).
There was a problem hiding this comment.
Which one do you refer by "we"?
To us as the framework developers&contributors to fulfill a currently existing requirement that caused this PR to be created (https://github.com/openhab/openhab2-addons/issues/3762).
but "me" rely on customized trustmanagers.
So you already have such a feature build for yourself? How are you using it without the framework offering such extension points?
There was a problem hiding this comment.
Note that the TrustManagerProvider returns a TrustManager given an endpoint.
If we want to replace the TrustManagerProvider it would make sense because it allows for an easy migration from the TrustManagerProvider to the new implementation.
There was a problem hiding this comment.
So you already have such a feature build for yourself? How are you using it without the framework offering such extension points?
How have bindings used HTTP clients before the ESH framework provides support for them?
They used Apache HTTP client of Jetty implementation without framework support.
There was a problem hiding this comment.
Note that the TrustManagerProvider returns a TrustManager given an endpoint.
Which should be deprecated and kept as it is - so I would suggest to use a different interface name here.
There was a problem hiding this comment.
They used Apache HTTP client of Jetty implementation without framework support.
@maggu2810 So you're saying that you would like to use the shared Jetty instance with that trust extension mechanism? Then sure, besides the certificate provider, we can also directly add an interface for custom trust managers, if this is what you'd additionally required. My point was only that there isn't any need to implement a feature that nobody has asked for yet.
| } | ||
| } | ||
|
|
||
| private X509ExtendedTrustManager getLinkedTrustMananger(X509Certificate[] chain) { |
There was a problem hiding this comment.
Note to self or someone that knows, I found another implementation which did not assume this chain to be ordered, this should of course be checked
|
I made a lot of changes including adding the two interfaces: I think the big picture should be there now, I also reverted the removal of the old logic. Now it needs to actually work ;-) And of course it will need some test coverage. |
|
@maggu2810 I wrote you a big question (below the ===), luckily while preparing a full explanation what went wrong I found the problem, I'm curious about the why now ;-) I solved the problems described below by changing: @Component(immediate = true)to @Component(service = ExtensibleTrustManager.class, immediate = true)======= for reference ==== The HttpUtil is not activated (anymore) I believe it also causes the test-error. Is see the following activated services when calling services on the osgi-prompt: However I see a log statement, which indicated that OSGI was busy constructing my To me the (generated) XML looks sensible: <?xml version="1.0" encoding="UTF-8"?>
<scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.0.0" immediate="true" name="org.eclipse.smarthome.io.net.http.internal.ExtensibleTrustManager">
<reference name="TlsCertificateProvider" interface="org.eclipse.smarthome.io.net.http.TlsCertificateProvider" cardinality="0..n" policy="dynamic" bind="addTlsCertificateProvider" unbind="removeTlsCertificateProvider"/>
<reference name="TlsTrustManagerProvider" interface="org.eclipse.smarthome.io.net.http.TlsTrustManagerProvider" cardinality="0..n" policy="dynamic" bind="addTlsTrustManagerProvider" unbind="removeTlsTrustManagerProvider"/>
<implementation class="org.eclipse.smarthome.io.net.http.internal.ExtensibleTrustManager"/>
</scr:component>I pushed all my current code to ESH to this branch, if needed I can push the implementation of the interface as well but I would hope that technically that would not be necessary looking at the If you have any tools / blogs to suggest which should properly debug activation problems then that would of course be great. TIA! |
|
|
||
| @Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC) | ||
| protected void addTlsTrustManagerProvider(TlsTrustManagerProvider tlsTrustManagerProvider) { | ||
| // TODO we could potentially overwrite existing implementation do we want to guard against that? |
There was a problem hiding this comment.
@kaikreuzer wdyt should there by any checks / warnings in case to Components implement a tls..Provider for the same hostname? Or should it even support it?
There was a problem hiding this comment.
I would log a warning and ignore any additional trust managers for urls for which we already have one. Just to prevent that during runtime someone might try to take over control of a specific url.
There was a problem hiding this comment.
Hm, if multiple trust managers for the same hostname are used, shouldn't we drop ALL the trust managers and log a big fat warning?
I assume we don't know which one is the correct one.
Perhaps the one that tries to take over the specific URL is loaded before the correct one after the next restart.
There was a problem hiding this comment.
It would feel wrong that a fully working system suddenly stops working, just because you have dynamically added another independent bundle that happens to use the same url. Imho a warning in that case is good enough and that's also how we proceed in all other cases where we expect a single service, but (dynamically) might get multiple.
There was a problem hiding this comment.
Hm, for me (personally) it feels more wrong to just write a log message? Who reads log messages if he think all is working fine?. If e.g. the windows defender identifies a potential security attack and write a log message I assume no one gets noticed ever.
Perhaps the situation is different here. I don't know.
If a binding communicates with a remote server and transfer security related things (isn't the most "home" automation more or less some very personal information) shouldn't we ensure that the certificate checking of a peer cannot be levered out easily? If an attacker can inject a TlsTrustManagerProvider and ensure it is loaded earlier then the correct one (for a specific hostname) shouldn't we be very noisy that there is something wrong (two providers for the same hostname)?
Sure, if someone can inject a HTTP client factory that is used instead of the framework provided one it is a more bad situation...
So perhaps a warning is enough as we cannot really identify such an attack...
@martinvw So, if I check out your branch and remove |
|
Reading the current implementation I would like to come up with a question.
If the consumer of the HTTP client factory and the provider of the TLS trust manager is contained in the same bundle we hope that the injection of the components will be faster then establishing the connection. Correct? |
Yes indeed, but it complies to what the docblock of the
So my class implements an interface ( Looking at the documentation this seems expected maybe it was just some OSGI ignorance from my side PS: if I have more time I will carefully review your question and will get back on it :-) |
|
@martinvw : do you plan in a next step to show how to use this stuff in a binding ? |
I'm not experienced enough with OSGI activation but I would expect that all components for a bundle are activated more or less at the same time and because IMHO the design is that normally the Tls*Provider and the consumer are from the same bundle, its likely that they will be activated at the same time, not necessarily the desired order.
Yes, but this already an existing design choice inside the io.net bundle, given that we require that the
Yes, but this was not an intended use case I had in mind but should of course work, the same as the in-bundle one.
If you have ideas for this that would be great 👍
What do you mean with this? |
There is the implementation in https://github.com/openhab/openhab2-addons/pull/3919/files I just pushed the newest version there. I hope to create tests which also should give some hints on the implementation and maybe I need to clarify some docblocks feel free to point out things where documentation or docblocks could be amended / improved. |
martinvw
changed the title
|
I added the most important tests, feel free to review if other things/tests are needed please let me know. |
| */ | ||
| package org.eclipse.smarthome.io.net.http; | ||
|
|
||
| import javax.net.ssl.X509ExtendedTrustManager; |
There was a problem hiding this comment.
@kaikreuzer @maggu2810 Is this javax a problem :'(
There was a problem hiding this comment.
At least also the following bundles contains a package import of javax.net.ssl.
- org.eclipse.jetty.io
- org.eclipse.jetty.jmx
- org.eclipse.jetty.server
- org.eclipse.jetty.util
The Jetty Util bundle is used by most of the Jetty bundles, so as long as Jetty needs it, it should not be a problem for us (IMHO).
But please don't miss do change the manifest, too (import package javax.net.ssl).
There was a problem hiding this comment.
Pff, I was afraid it was all in vain :-) Thanks!!!
There was a problem hiding this comment.
I do not understand. Is javax a bad namespace? All the ssl goodies are in there.
There was a problem hiding this comment.
I mistakenly assumed that javax was completely not a part of compact profile, but indeed some of the Javax packages are part of the compact profile 👍
martinvw
changed the title
|
Resolved the conflicts :-) |
|
@kaikreuzer / @maggu2810 it would be great if you have time to review :-) |
There was a problem hiding this comment.
Many thanks @martinvw, great work!
I give @maggu2810 a chance to review/comment as well - if there's no veto by tomorrow, I'll merge 👍.
...smarthome.io.net/src/main/java/org/eclipse/smarthome/io/net/http/TlsCertificateProvider.java
Outdated
Show resolved
Hide resolved
...smarthome.io.net/src/main/java/org/eclipse/smarthome/io/net/http/TlsCertificateProvider.java
Outdated
Show resolved
Hide resolved
...marthome.io.net/src/main/java/org/eclipse/smarthome/io/net/http/TlsTrustManagerProvider.java
Outdated
Show resolved
Hide resolved
...marthome.io.net/src/main/java/org/eclipse/smarthome/io/net/http/TlsTrustManagerProvider.java
Outdated
Show resolved
Hide resolved
...marthome.io.net/src/main/java/org/eclipse/smarthome/io/net/http/TlsTrustManagerProvider.java
Outdated
Show resolved
Hide resolved
....io.net/src/main/java/org/eclipse/smarthome/io/net/http/internal/ExtensibleTrustManager.java
Outdated
Show resolved
Hide resolved
...me.io.net/src/main/java/org/eclipse/smarthome/io/net/http/internal/WebClientFactoryImpl.java
Show resolved
Hide resolved
|
@maggu2810 thanks for your valuable comments, I will try to process them later this week! |
This deprecates the TrustManagerProvider Its actually a composite TrustManager consisting of other TrustManagers (and the default fallback) and forwarding calls to an implementation based on the common name or subject alternative names. Fixes: #6196 Signed-off-by: Martin van Wingerden <martin@martinvw.nl>
|
@maggu2810 I was able to do the easy things, I will hopefully look at the rest in the upcoming days, but they might be busy |
- Extracted parent interface - Marked osgi setter/getter deprecated - Added some missing documentation Signed-off-by: Martin van Wingerden <martin@martinvw.nl>
|
@martinvw Are you done with your updates? If so, please ask @maggu2810 to do the final review/approval! |
|
No not yet, thanks for checking on me! |
...est/src/test/java/org/eclipse/smarthome/io/net/http/internal/ExtensibleTrustManagerTest.java
Show resolved
Hide resolved
...me.io.net/src/main/java/org/eclipse/smarthome/io/net/http/internal/WebClientFactoryImpl.java
Show resolved
Hide resolved
|
I have my own build errors.... |
|
Yeah, see openhab/openhab-distro#806 (comment). |
The main difference I see is the QA profile and jaccoco (which is very old version) I'm trying to reproduce it locally, but those builds take ages ;-) |
Locally on my Mac no-problem on my server I accidentally have installed Java 10 so have to downgrade first :-) Can you easily see the maven version somewhere? |
|
@pfink / @kaikreuzer looking at the date it looks like the openhab-sandbox build might be impacted but the errors looks different or is that because its a pipeline? |
....net.test/src/test/java/org/eclipse/smarthome/io/net/http/internal/WebClientFactoryTest.java
Show resolved
Hide resolved
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
|
@martinvw : I am a little lost. If I correctly understand, I just have to implement the interface TlsCertificateProvider and declare it as a component (service) and that's all ? What's the difference between TlsCertificateProvider and TrustManagerProvider ? |
|
In fact, Free is providing two certificates, one using the ECDSA algorythm and another using the RSA algorythm; Apparently, ECDSA is recenter and was introduced with TLS specification v1.2. I am not sure I really need to use the two certificates. My question is simple: does HttpUtil.executeUrl use ECDSA or RSA ? This certainly leads to the question for the underlying Jetty HTTP client ? |
|
All recent Java 8 versions support ECDSA, so just go for that. One should then be enough. Run time injection might need some small changes, but that should be covered in the mentioned PR 🙂 |
|
I will ckeck the common name set for the 2 certificates and compare it with the dynamically provided hostname. Maybe they are identical and then I could set the hostname hardcodedlike for the iCloud binding. |
|
The two certificates are apparently not defining a specific domain: I have currently the two certificats added in my cacerts on my RPI, so maybe the 2 were required. To be confirmed. The hostname/port used for connection is finally something like |
|
It looks like I can get something working well with SSL in Eclipse on my PC with only one crt file + a FreeboxTlsCertificateProvider class/service using it and returning my specific In theory, the binding should be able to handle several Freebox servers and so one could require xxxx.fbxos.fr while the other could require yyyy.fbxos.fr. So I really thing that support for wildcard in TlsProvider would be the solution. Is it doable ? |
|
Or should I register FreeboxTlsCertificateProvider services dynamically with correct host names ? |
From a performance point of view that would indeed be ideal, but I fear that the current ESH code does not properly support that yet |
|
Are you sure ? I have the feeling you have handled dynamic adding of new service. |
|
Yes but not with a hostname from configuration, I’ll try to get on back on this in the upcoming days |
|
The link between the hostname and the certificate provider is currently set only when the service is created. A configuration setting to update the hostname would probably allow updating it later. |
|
Thinking again about it, I think we are going in the wrong direction. Defining a static service like in the iCloud binding would be enough for me if I could use wildcard in hostname to link the certificate not only to a specific domain but to all sub-domains of a domain. Even if I can't find the valid domain names for the Freebox certificates, I am almost sure that a DNS of the kind example.fbxos.fr is associated to each freebox and the 2 certificates are valid for *.fbxos.fr. |
|
@martinvw : I found a solution without changing anything in ESH. |
|
Great! |
It was deprecated since the introduction of the ExtensibleTrustManager (eclipse-archived/smarthome#6281). Related to openhab#1408 Signed-off-by: Wouter Born <github@maindrain.net>
* Remove deprecated `TrustManagerProvider` It was deprecated since the introduction of the `ExtensibleTrustManager` (eclipse-archived/smarthome#6281). Related to #1408 Signed-off-by: Wouter Born <github@maindrain.net>
This replaces the TrustManagerProvider
Its actually a composite TrustManager consisting of other TrustManagers (and the default fallback) and forwarding calls to an implementation based on the common name or subject alternative names.
Fixes: #6196