Device Registry HTTP API #1130
Device Registry HTTP API #1130
Conversation
ctron
force-pushed
the
feature/devreg_api_1
branch
2 times, most recently
from
1738ea2
to
1235e2a
Compare
|
I added the credentials API, to the best of my understanding. I also started to work on a revised API, the current process is focused on the basics of tenant + registration. The main changes are:
This is not a final version, but a work in progress in order to discuss and refine it further. Feedback is welcome! Points to discuss:
Inspiration came from: |
|
I am not sure about dropping the ID from the payload : having tthe AMQP and the HTTP APIs aligned is a nice feature imo. |
|
I like the improvements so far! |
|
In fact, allowing the user to select a (potentially weak) hash function himself looks like a mistake to me in hindsight. I would therefore propose to not allow the user to provide the hashed password anymore at all but require him/her to provide the plain text password instead so that the registry can enforce proper usage of a strong hash function ... |
I fully agree. I think the device registry should be configured, by the administrator, to use whatever they think is reasonable. And then let them enforce the use of specific hash algorithm and salt. As right now you can still use an un-salted hash. |
So I think the APIs for AMQP and HTTP service different purposes. And have different behaviors. For HTTP, you have the ID in the URL/path. Having them, in addition, in the payload only makes things confusing IMHO. Currently the device registry actually removes them before processing internally. So I think it should only live in one play, and for a RESTful API, that sounds like the path to me. |
|
+1 for removing the client-side hash support 👍 Transmitting a hashed password in the payload doesn't add any security anyway.
you're right, I did not notice the ID was already in the URL, so yeah that make sense to remove it from the payload. |
|
Thanks for putting up the swagger UIs 👍 It's really convenient to review the API this way. POST /tenants/{tenant}
GET /tenants/{tenant}
PUT /tenants/{tenant}
POST /registrations/{tenant}/{deviceId}
GET /registrations/{tenant}/{deviceId}
|
Currently it is required. But indeed, it might be a good idea to have an automatically generated tenant ID. If that is what you meant.
Yes, there is no payload, but the request could be invalid otherwise. e.g. when we have something like paging, (start > end -> invalid request).
Great idea!
The suggestions I read many times is to simply always use the plural form. I kept registration as I wanted to be close the current. But renaming this to "devices" would make absolute sense to me. |
ctron
force-pushed
the
feature/devreg_api_1
branch
3 times, most recently
from
371e7b4
to
8b96cd0
Compare
|
Ok, I pushed a change. This should cover most of the feedback. |
|
I thought a couple of times about changing of registrations to devices and now might be the right time to do it. Hierarchy tenants->devices->credentials look much clearer IMHO |
|
Looks good, just one more thing to fix. Clients need to specify the expected version of the resource in a PUT request in the If-Match header, not the ETag header (that is only used to convey the version of the resource in a response). |
ctron
force-pushed
the
feature/devreg_api_1
branch
2 times, most recently
from
d1a7991
to
b5d42e1
Compare
|
I didn't know about I will move on then to the credentials section. |
So, you would only want to delete the object if its version was |
Correct. The reason for this would be to detect/prevent and accidental deletion of a re-created device. Assuming two operations in parallel: a) delete device "A", create device "A" Depending on the order of operations the outcome might be different each time. And the intention of deleting "A" in b) would be based on the original "A", not the re-created "A". If the user doesn't care in b), then the header parameter can simply be omitted and the re-created A gets deleted, but if the user cares, then there would be a way now. |
|
Hi, thank you very much for the API draft. Having a well defined Hono rest API is something I always wanted :-) One of the big benefits for such I see for writing web UIs for device / tenant management. For such operations one method however I sill would wish to be well-defined as-well: A list operation on the devices (and probably for the tenants). I already implemented such method as a draft for our hono deployment (with pagination / querying). If you're interested I could post our signature here. Thanks |
Yes, I think that is a good idea. If you have something already, I would be glad to take a look at this. |
|
Hi, I'm currently a bit busy, so here the api draft just as text (and not as a nice openapi doc): GET call on Query Parameters:
Example: Response:
|
I am a little more reluctant than @ctron. The cons listed in the provided link pose some interesting questions. Do we expect to impose some particular order for the objects, e.g. based on ID? That would probably alleviate the issue a little. And before the question comes up: I am quite strictly opposed to having a query API in addition to get by ID .... :-) |
| $ref: '#/components/responses/Unauthorized' | ||
| 403: | ||
| $ref: '#/components/responses/NotAllowed' | ||
| 409: |
There was a problem hiding this comment.
if no read access it should answer 404 as well.. Having 403 to mask conflict and 404 to mask 403 allows someone to poke around and find outs existing tenants ID, if i am correct.
| - tenants | ||
| summary: Create new tenant | ||
| operationId: createTenantWithId | ||
| parameters: |
There was a problem hiding this comment.
Even thought this naming is valid in terms of rest and openapi in general, its problematic for most of the open-api code generators(at least the java once). Since both, the path-parameter and the body are called "tenant", the generated interfaces would have duplicate variable names in the updateTenant and createTenantWithId methods.
I would suggest to call the pathParameter something like tenantName or tenantId (similar to deviceId in the DevicesApi) to avoid such problems.
There was a problem hiding this comment.
Interesting point, I just tried that:
$ npx @openapitools/openapi-generator-cli generate -i device-registry-revised.yaml -g java -o build/
npx: installed 1 in 1.252s
Exception in thread "main" org.openapitools.codegen.SpecValidationException: There were issues with the specification. The option can be disabled via validateSpec (Maven/Gradle) or --skip-validate-spec (CLI).
| Error count: 9, Warning count: 0
Errors:
-attribute paths.'/devices/{tenant}/{deviceId}'(put).parameters.There are duplicate parameter values
-attribute paths.'/credentials/{tenant}/{deviceId}'(get).parameters.There are duplicate parameter values
-attribute paths.'/devices/{tenant}/{deviceId}'(delete).parameters.There are duplicate parameter values
-attribute paths.'/devices/{tenant}/{deviceId}'(get).parameters.There are duplicate parameter values
-attribute paths.'/credentials/{tenant}/{deviceId}'(put).parameters.There are duplicate parameter values
-attribute paths.'/credentials/{tenant}/{deviceId}'(delete).parameters.There are duplicate parameter values
-attribute paths.'/tenants/{tenant}'(put).parameters.There are duplicate parameter values
-attribute paths.'/tenants/{tenant}'(delete).parameters.There are duplicate parameter values
-attribute paths.'/devices/{tenant}/{deviceId}'(post).parameters.There are duplicate parameter values
at org.openapitools.codegen.config.CodegenConfigurator.toClientOptInput(CodegenConfigurator.java:626)
at org.openapitools.codegen.cmd.Generate.run(Generate.java:367)
at org.openapitools.codegen.OpenAPIGenerator.main(OpenAPIGenerator.java:60)
So that is indeed something that needs fixing. Thanks!
There was a problem hiding this comment.
Ok, I did clean up the parameter definitions. However I think the remaining validation errors are due to:
- [BUG] Validation error: "There are duplicate parameter values" OpenAPITools/openapi-generator#2631
- Update swagger-parser-version to 2.0.13-OpenAPITools.org-1 OpenAPITools/openapi-generator#2775
However that shouldn't be a real problem. I will check the body issue now.
There was a problem hiding this comment.
Ok, I see your point now. Code generation is always fun 🙄
| "psk": '#/components/schemas/PSKSecret' | ||
| "x509-cert": '#/components/schemas/X509CertificateSecret' | ||
|
|
||
| CommonSecret: |
There was a problem hiding this comment.
Maybe I'm missing something, but I think the CommonSecret should extend the TypedSecret.
E.g.:
CommonSecret:
allOf:
- $ref: '#/components/schemas/TypedSecret'
- type: object
properties:
"type":
type: string
"enabled":
type: boolean
default: true
"not-before":
type: string
format: date-time
"not-after":
type: string
format: date-time
"comment":
type: stringAgain, in terms of code generation for java, this would lead to a decoupled TypedSecret and would be hard to get back together with the concrete secrets. Currently you would get something like:
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type", visible = true)
@JsonSubTypes({
@JsonSubTypes.Type(value = PasswordSecret.class, name = "hashed-password"),
@JsonSubTypes.Type(value = PSKSecret.class, name = "psk"),
@JsonSubTypes.Type(value = X509CertificateSecret.class, name = "x509-cert"),
})
public class TypedSecret { ... }
public class CommonSecret { ... }
public class X509CertificateSecret extends CommonSecret { ... }where the "oneOf" information is (in best case) included as annotations, but not in the object hirarchy. The example is for spring(with jackson), if you use the plain java generator (or f.e. the vertx-generator) you get no connection between TypedSecret and Common/PasswordSecret/etc. at all.
There was a problem hiding this comment.
I tried to explain this in previous comment. The idea was the following (and I didn't check for the generated code, but then again, code generation always misses something):
Each device has a list of authentication identities, which is defined as a Map<String,AuthenticationIdentity>. Each AuthenticationIdentity has a list of secrets, which could either be PasswordSecret, PSKSecret, or some other type.
So the AuthenticationIdentity should look like:
class AuthenticationIdentity {
List<TypedSecret> secrets;
}The TypedSecret is the "union type" for a single secret entry. It is required to have type, but none of the other properties from CommonSecret.
The other secrets however share the fields from the CommonSecret, just because the choose to do so.
So again, looking at Java I would see this as:
interface TypedSecret {
String getType();
String setType(String type);
}
class CommonSecret {
public String getType() {
return type;
}
public void setType(String type) {
this.type = type;
}
// ...
}
class PassswordSecret extends CommonSecret {
}My experience with the code generators from OpenAPI and Swagger showed that you will always have something that doesn't really work. And if it works fine in language A, it fails in language B. So, I am not sure how far we should push the spec file to produce code. I would see it more as a documentation.
There was a problem hiding this comment.
But just to be clear. I am open to suggestions on how to improve this!
There was a problem hiding this comment.
Thanks for the explanation. It seems that this is a shortcoming of the generators, it makes totally sense the way you defined it.
There was a problem hiding this comment.
@wistefan I am glad to hear that. I did in the meantime play around with the model, based on your proposal and what OpenAPI documents. I ended up with something that worked fine-ish in Java, was messed up both in Go and the actual Specification then (and I didn't test beyond that).
I guess we have to still wait a few centuries, for a modelling approach that just works 😉
So for the moment I would focus in the spec itself, and less on the code generation.
There was a problem hiding this comment.
And just because I didn't say it explicitly. I am thankful for your input! Keep it coming!
There was a problem hiding this comment.
Especially for java, there are quite some discussions on how to implement the support for one-of(and for that matter all-of and any-of) in the generators. I extended my generator now to produce it as:
interface TypedSecret {
String getType();
String setType(String type);
}
class CommonSecret {
public String getType() {
return type;
}
public void setType(String type) {
this.type = type;
}
// ...
}
class PassswordSecret extends CommonSecret implements TypedSecret {
}which seems to work best at the moment.
I also think it will take a while to for a working modelling approach(if there will be one at all).
|
I added a note about the non-focus on code generators. If you think that is too grumpy, just let me know. |
|
|
||
| # Common schema | ||
|
|
||
| Error: |
There was a problem hiding this comment.
Since you already define a specific error response object, I think it would be good to include a (optional) error-code field. A frontend might use it to show localized messages or an automated client might use it for a more specific errorhandling.
Beside that, I mostly finished an implementation of this api and I really like it👍
There was a problem hiding this comment.
@wistefan which parts did you implement ? I wanted to start this work during this week but there is no need to duplicate the effort if you've worked on it. Would you like some help ?
There was a problem hiding this comment.
The whole Rest-Part of the api, including persistence in a sql-database. Since I implemented it using micronaut I'm not totally sure if it will fit into the whole hono project and I still have some work to do until I can put it on github(f.e. resolving the dependencies to our internal openapi-generator). It will probably take a couple of weeks(~4) until I can prepare a PR.
There was a problem hiding this comment.
So you did not update the HttpEndpoint classes ?
ctron
force-pushed
the
feature/devreg_api_1
branch
from
fd66fb7
to
6c4eecf
Compare
|
@sophokles73 I made the change requested by you of putting the "auth-id" explicitly into the secrets as a field. Please have a look if that is ok for you now. |
|
I've started to work on implementing this in #1240 . I noticed two things on the tenant API :
|
For the
The versioning is indeed optional. If you don't provide it in a modifying operation, the version check will be skipped.
Looks like it, I will check again. |
After reading through it once again, I think it is complicated 😁 So yes, the scenario you described would give out the information if the tenant exists or not. I was assuming that the user either has "create tenant" access, in which case you would get Now you could have the situation that a user has "create tenant" access, but doesn't have access for this specific tenant. Honestly I don't know what to do in that case, because the tenant name is a unique element, and you simply have to reject the request. On the other hand I think it is fair to say that, if the user has a global "create tenant" access role, then he can also know if that specific tenant exists or not. He still woulnd't see the content of that tenant though. |
|
Thanks for your answers @ctron, that clears a lot of things for me 👍
That sounds like a reasonable asumption to make. Maybe that could be enforced in the auth service ? i.e. having a hierarchy of roles and one cannot have create access without read, so such situations are avoided (unix permission model works great for that) |
| properties: | ||
| "type": | ||
| type: string | ||
| "not-before": |
There was a problem hiding this comment.
I do understand your intention but I strongly disagree with the approach taken.
- The auth-id is actually one of the most important properties when it comes to authenticating a device. The fact that the schema does not define nor mention it in any place, makes it close to impossible to understand the structure of the data without reading prose in the form of comments (which are lacking).
- The authentication identifier may be shared by multiple secrets but it does not have to be shared. Thus, using it as the key in the secrets map seems arbitrary to me. In fact, devices will most probably not have dozens of secrets defined for them but will more likely have 1 to 5 of them. Extracting the auth-id in order to allow for quicker look up (if that is indeed the motivation) will probably not make much difference. However, not having an explicit auth-id property within the secret, makes the whole thing harder to read and understand FMPOV.
I would therefore much prefer to have the CommonSecret explicitly define the auth-id property and refrain from using the auth-id implicitly as the key in a map of secrets, but instead simply contain the secrets in an array of CommonSecrets ...
| title: Eclipse Hono™ Device Registry API | ||
| description: | | ||
| This API allows one to interact with the Eclipse Hono Device Registry | ||
| API. It acts as a common basis which all Hono device registries should |
There was a problem hiding this comment.
there is no such thing as the Device Registry API. Instead, we have Tenant, Device Registration and Credentials APIs.
| the specification in a way to please code generators. | ||
|
|
||
| contact: | ||
| email: "hono-dev@eclipse.org" |
There was a problem hiding this comment.
this seems a little problematic, as you can only send to this address, if you have subscribed to the list ...
There was a problem hiding this comment.
I am open to suggestions.
There was a problem hiding this comment.
I would replace the email property with a url property pointing to https://www.eclipse.org/hono/community/get-in-touch/
| "comment": | ||
| type: string | ||
|
|
||
| X509CertificateSecret: |
There was a problem hiding this comment.
can this secret have additional properties?
There was a problem hiding this comment.
Not to my knowledge. However it is a distinct type via the "discriminator" value in TypedSecret.
There was a problem hiding this comment.
I was wondering, if the way that the object is defined, would allow clients to set additional properties on it ...
There was a problem hiding this comment.
Ok, now I understand what you meant. Done.
| allOf: | ||
| - $ref: '#/components/schemas/CommonSecret' | ||
|
|
||
| PasswordSecret: |
There was a problem hiding this comment.
can this secret have additional properties?
| type: string | ||
| format: byte | ||
|
|
||
| PSKSecret: |
There was a problem hiding this comment.
can this secret have additional properties?
ctron
force-pushed
the
feature/devreg_api_1
branch
2 times, most recently
from
3cfd6d2
to
a6e1644
Compare
| 401: | ||
| $ref: '#/components/responses/Unauthorized' | ||
| 403: | ||
| $ref: '#/components/responses/NotAllowed' |
| "comment": | ||
| type: string | ||
|
|
||
| X509CertificateSecret: |
There was a problem hiding this comment.
I was wondering, if the way that the object is defined, would allow clients to set additional properties on it ...
ctron
force-pushed
the
feature/devreg_api_1
branch
2 times, most recently
from
7961987
to
a6b5d78
Compare
No, before merging, I would like to drop the "old" API spec, and squash the PR. I would still recommend to keep the existing API implemented for a release or two. Just to let people continue to work with existing documentation and not break those. |
ctron
force-pushed
the
feature/devreg_api_1
branch
2 times, most recently
from
037529e
to
b14679f
Compare
| put: | ||
| tags: | ||
| - credentials | ||
| summary: Update credentials set for registered device |
There was a problem hiding this comment.
The only way to add a credentials seems to be getting all credentials, then modifying, deleting or adding an entry and then putting the result back. This requires some client side logic for JSON parsing to find the right entry. Dedicated URLS for putting and deleting an entry would add much more convenience for the client. A use case like credential rotation can then be implemented without any parsing.
PUT /credentials/{tenantId}/{deviceId}/{auth-id}
DELETE /credentials/{tenantId}/{deviceId}/{auth-id}
Updating a set at once (PUT /credentials/{tenantId}/{deviceId}}) may not be needed when individual URLs are provided.
Deleting all at once would still be convenient for cleaning up all credentials at once.
Was the option of providing dedicated auth id update considered?
This is just an API consumers view, there might be some reasons internally, why this isn't a good idea :-)
There was a problem hiding this comment.
makes sense to me. However, the resources would need to include the credentials type as well, i.e.
PUT /credentials/{tenantId}/{deviceId}/{type}/{auth-id}
DELETE /credentials/{tenantId}/{deviceId}/{type}/{auth-id}
There was a problem hiding this comment.
Was the option of providing dedicated auth id update considered?
However, the resources would need to include the credentials type
That is how the current API look like : https://current-api-hono-device-registry-api.wonderful.iot-playground.org/
I am not sure why those operation have been removed. Also : how does one create a credential if none exist yet ?
There was a problem hiding this comment.
I touched that point in the following comment: #1130 (comment)
I agree, I think it makes sense to have such specialized operations. However they can all be implemented on top of this API. And I think it makes sense to add them in a later version of the API. However I don't think it conflicts with the current API definition.
Assuming you have all kinds of specialized secrets (Password, PSK, client cert, custom protocol adapter secrets) I think it makes sense to support them in the way currently described. And then adding more specific operations on top of it, like:
- Cleanup expired passwords
- Password / Key rollover
- …
So, let's finish this first. And then do version 1.1, including search and specialized secret operations.
This change adds an OpenAPI v3 definition of the device registry API, used for managing tenants, devices and credentials. It is an addition to the existing AMQP 1.0 API, but is intended to be used to provide common tooling for managing devices in a Hono compatible device registry.
ctron
force-pushed
the
feature/devreg_api_1
branch
from
c7f18d1
to
eb0a3b4
Compare
Documented:
This PR contains an OpenAPI 3 model, which can be rendered and tested using the following command:
Or directly from GitHub (without the option to make local changes):
You can also directly use the hosted version here:
Current API – https://current-api-hono-device-registry-api.wonderful.iot-playground.orgRevised API – https://revised-api-hono-device-registry-api.wonderful.iot-playground.orgYou can actually use the "Try it out" feature, as currently the API describes the "as is" situation. However I would like to change this for the final version. You can already see in the definition why, but let's wait with the discussion until all the current APIs are mapped.