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

[#1296] Add feature auto provisioning #1631

Merged
merged 1 commit into from Dec 12, 2019

Conversation

@b-abel
Copy link
Contributor

b-abel commented Nov 22, 2019

This adds minimal support for automatic device provisioning (#1296) when authenticating with X509 client certificates. Support is added for a flag in the client configuration that allows automatic provisioning to be enabled or disabled for each trusted CA configured. When a device connects to a protocol adapter, the adapter checks whether the flag for the trust anchor used is set to true. If so, it places the serialized device certificate in the payload of the GET request to the Credentials API. This way, the device registry has all the information available to perform automatic provisioning if necessary.

@b-abel b-abel requested review from ctron, dejanb and sophokles73 as code owners Nov 22, 2019
@b-abel

This comment has been minimized.

Copy link
Contributor Author

b-abel commented Nov 22, 2019

I will add documentation and tests when we have agreed on the implementation.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 22, 2019

Codecov Report

Merging #1631 into master will increase coverage by 0.17%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1631      +/-   ##
============================================
+ Coverage     66.98%   67.15%   +0.17%     
  Complexity      652      652              
============================================
  Files           350      351       +1     
  Lines         15317    15360      +43     
  Branches       1316     1317       +1     
============================================
+ Hits          10260    10315      +55     
+ Misses         4129     4118      -11     
+ Partials        928      927       -1
Impacted Files Coverage Δ Complexity Δ
...ava/org/eclipse/hono/client/CredentialsClient.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...clipse/hono/client/impl/CredentialsClientImpl.java 86.79% <ø> (ø) 0 <0> (ø) ⬇️
...ono/client/impl/AbstractRequestResponseClient.java 74.15% <ø> (ø) 0 <0> (ø) ⬇️
...va/org/eclipse/hono/util/CredentialsConstants.java 60% <ø> (ø) 0 <0> (ø) ⬇️
...in/java/org/eclipse/hono/util/TenantConstants.java 90% <ø> (ø) 0 <0> (ø) ⬇️
...se/hono/service/auth/device/DeviceCredentials.java 0% <0%> (ø) 0 <0> (?)
...ervice/auth/device/CredentialsApiAuthProvider.java 83.33% <100%> (ø) 0 <0> (ø) ⬇️
.../main/java/org/eclipse/hono/util/TenantObject.java 84.73% <100%> (+1.82%) 0 <0> (ø) ⬇️
...hono/service/auth/device/SubjectDnCredentials.java 100% <100%> (ø) 0 <0> (ø) ⬇️
...service/auth/device/AbstractDeviceCredentials.java 100% <100%> (ø) 0 <0> (ø) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f59b10f...7311f0c. Read the comment docs.

@b-abel b-abel requested a review from calohmn as a code owner Nov 26, 2019
@b-abel

This comment has been minimized.

Copy link
Contributor Author

b-abel commented Nov 29, 2019

I applied the rested changes and added unit tests.

@b-abel

This comment has been minimized.

Copy link
Contributor Author

b-abel commented Nov 29, 2019

I will add documentation.

I'm thinking about adding support for this feature to the file-based device registry if this is not too much effort. This could serve as an implementation example and it would make it relatively easy to provide integration tests for this feature.
WDYT?

Copy link
Member

sophokles73 left a comment

LGTM

@sophokles73 sophokles73 added this to In progress in 1.1.0 via automation Dec 9, 2019
@sophokles73 sophokles73 added this to the 1.1.0 milestone Dec 9, 2019
@sophokles73

This comment has been minimized.

Copy link
Member

sophokles73 commented Dec 9, 2019

@b-abel do you want to add documentation to this PR or create another one for it?

@b-abel

This comment has been minimized.

Copy link
Contributor Author

b-abel commented Dec 9, 2019

I am currently working on the documentation and would rather add it in this PR.

@b-abel

This comment has been minimized.

Copy link
Contributor Author

b-abel commented Dec 9, 2019

I added a small enhancement to the "Credentials API": the "Get" request can now return the status code 201. The documentation of the Credentials API states: "The response message’s status property may contain the following codes". So, I would like to explicitly add code 201 here. Which is FMPOV perfectly fine for a new minor version.
The CredentialsClientImpl now treats it the same way as a 200, so this makes no difference from the protocol adapters POV, but it allows to extend device registries and allow for cleaner APIs without breaking the client implementation.
WDYT?

@sophokles73

This comment has been minimized.

Copy link
Member

sophokles73 commented Dec 9, 2019

@b-abel I cannot find any change to the Credentials API regarding the new status code ...

@b-abel

This comment has been minimized.

Copy link
Contributor Author

b-abel commented Dec 9, 2019

@sophokles73 I only committed the change in the client. But it might be easier to follow with documentation, so I will add it as well.

@sophokles73

This comment has been minimized.

Copy link
Member

sophokles73 commented Dec 9, 2019

@b-abel IMHO it should be the other way around: first we specify the intended behavior by means of the API, then we implement the spec and create clients for it :-)

@b-abel

This comment has been minimized.

Copy link
Contributor Author

b-abel commented Dec 9, 2019

@b-abel IMHO it should be the other way around: first we specify the intended behavior by means of the API, then we implement the spec and create clients for it :-)

D'accord. Currently (Hono 1.0.x) the Credentials API does not restrict the status codes in response to the "get" operation, but the client (CredentialsClient / CredentialsClientImpl) treat the codes 201-299 as errors. I think we could at least support 201 on the client-side in Hono 1.1.

@b-abel

This comment has been minimized.

Copy link
Contributor Author

b-abel commented Dec 9, 2019

Regarding the documentation: Is this enough? Or do we additionally want to document it in the user guide for each protocol adapter? Or do we even need a central place to describe the whole idea (e.g. in concepts)?

@@ -107,6 +107,17 @@ paths:
example:
error: "Root Certificate Authority is already used by other tenant"
subject-dn: "CN=devices,OU=iot,O=ACME"
501:

This comment has been minimized.

Copy link
@sophokles73

sophokles73 Dec 9, 2019

Member

I wonder if using 501 here matches the code's intended usage (from the HTTP 1.1 spec):

The server does not support the functionality required to fulfill the request. This is the appropriate response when the server does not recognize the request method and is not capable of supporting it for any resource.

In this case the server (the registry is very well able to process the POST request to the tenant resource. However, the values in the payload are not acceptable for the registry. IMHO a 400 would be a better match here, including a description of the problem in the payload.
@ctron @dejanb @calohmn any thoughts?

This comment has been minimized.

Copy link
@b-abel

b-abel Dec 10, 2019

Author Contributor

If we use a more generic code like 400, I would not document it in the swagger file. So I would remove it from the yaml if we go for 400. WDYT?

This comment has been minimized.

Copy link
@sophokles73

sophokles73 Dec 11, 2019

Member

Works for me.

This comment has been minimized.

Copy link
@sophokles73

sophokles73 Dec 11, 2019

Member

Are you going to remove the 501 then?

site/homepage/content/release-notes.md Outdated Show resolved Hide resolved
site/homepage/content/release-notes.md Outdated Show resolved Hide resolved
@b-abel

This comment has been minimized.

Copy link
Contributor Author

b-abel commented Dec 10, 2019

I added a concept page for this feature. If you approve it, I would not add it to the user guides of the protocol adapters. I do not see what should go there as the idea is that the device does not need to know if it is registered.

Copy link
Member

sophokles73 left a comment

I am a little concerned that readers will have a hard time understanding what this is all about without some context being provided.
IMHO we would be better off with a concept page for Device Provisioning in general which explains

  • what does provisioning a device mean and why is it necessary?
  • what information needs to be provisioned for a device to be usable in Hono?
  • how can you actually provision a device?
    ** Manually, using the Device Management API, or
    ** automatically using auto-provisioning

We could then also move the second paragraph of Device Registration to this new concept page.
WDYT?

@b-abel

This comment has been minimized.

Copy link
Contributor Author

b-abel commented Dec 10, 2019

@sophokles73 IMHO the page Device Identity and the user guide of the device registry (together with the management API specification) currently describe device provisioning. Therefore it is a little difficult for me not to produce too much overlap. Please find my proposal in the last commit.

site/homepage/content/release-notes.md Outdated Show resolved Hide resolved
@@ -107,6 +107,17 @@ paths:
example:
error: "Root Certificate Authority is already used by other tenant"
subject-dn: "CN=devices,OU=iot,O=ACME"
501:

This comment has been minimized.

Copy link
@sophokles73

sophokles73 Dec 11, 2019

Member

Works for me.

site/documentation/content/api/credentials/index.md Outdated Show resolved Hide resolved
@@ -107,6 +107,17 @@ paths:
example:
error: "Root Certificate Authority is already used by other tenant"
subject-dn: "CN=devices,OU=iot,O=ACME"
501:

This comment has been minimized.

Copy link
@sophokles73

sophokles73 Dec 11, 2019

Member

Are you going to remove the 501 then?

site/homepage/content/release-notes.md Outdated Show resolved Hide resolved
@b-abel

This comment has been minimized.

Copy link
Contributor Author

b-abel commented Dec 11, 2019

@sophokles73 Since there have been no further comments by others, I would remove the error codes 501 from the management API. This means that we leave the behavior of Device Registries that do not support auto-provisioning completely unspecified, right? FMPOV this is OK.

@b-abel

This comment has been minimized.

Copy link
Contributor Author

b-abel commented Dec 11, 2019

I added the requested changes.

@sophokles73

This comment has been minimized.

Copy link
Member

sophokles73 commented Dec 11, 2019

@b-abel ok, can you squash and rebase?

Auto-Provisioning is documented in concept page *Device Provisioning*.
In the tenant configuration a new flag is added to enable or disable
auto-provisioning for each configured trusted CA.

If auto-provisioning is enabled for the trust anchor of the client
certificate used by a device for authenticating, the serialized device
certificate is put into the payload of the GET request to the Credentials
API to enable auto-provisioning in the Device Registry.

The GET request to the Credentials API can now return the status code 201.
The client implementation handles it in the same way as status code 200.

Signed-off-by: Abel Buechner-Mihaljevic <Abel.Buechner@bosch-si.com>
@b-abel b-abel force-pushed the bosch-io:PR/#1296_auto-provisioning branch from a6f23db to 7311f0c Dec 11, 2019
@sophokles73 sophokles73 merged commit 8cf4de5 into eclipse:master Dec 12, 2019
4 checks passed
4 checks passed
codecov/patch 88.88% of diff hit (target 66.98%)
Details
codecov/project 67.15% (+0.17%) compared to f59b10f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
eclipsefdn/eca The author(s) of the pull request is covered by necessary legal agreements in order to proceed!
Details
1.1.0 automation moved this from In progress to Done Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
1.1.0
  
Done
2 participants
You can’t perform that action at this time.