Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
gcoap: add DTLS integration #12104
gcoap: add DTLS integration #12104
Changes from 13 commits
73bd7e4
0ec019c
667fea8
0e32a96
35a7316
84d99ac
0f38ac5
70315bd
02981c7
c52b34b
93f8e98
941dace
236732f
8b39c97
c20a4d7
9921a22
dfdbb86
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you choose 10 for this value? Why not 1 or 5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be any
unsigned
values. No reason specifically why 10 is chosen here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More importantly, why define this value here? If it was defined in
gcoap.c
orgcoap.h
, you would not needgcoap_set_credential_tag()
, right? Is there any value to defining it in the application? Would gcoap ever need more than one credential? Do we need a distinction between a gcoap server credential and a client credential?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following this line of thinking, why not move all of the code from this PR in
gcoap_cli.c
into gcoap itself? gcoap provides CoAP as a service, so why should every application need to add this code? With this approach the application/user only needs to providecredentials.c
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, you're right. The credential tag is defined in the client to avoid reserving values for the tag used in gcoap. Other modules (e.g. LWM2M) might also hard-code the tag used for the DTLS sock and then there will be a need to track the available tag somewhere to avoid collisions.
Yes. gcoap might be talking to other secure coap clients/servers that use different credentials.
No.
The credentials are added in
gcoap_cli.c
because we do not want to make any assumption or limit what the name of the variables used for the credentials should be.In general, the design of coupling DTLS sock with credman is to avoid limiting what the name of the credentials should be or where it can be found. This is left to the user to define.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth specifically mentioning configuration parameters here. I got hung up for a while on the default value of 1 for tinydtls DTLS_PEER_MAX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expected to see a comment similar to what was in
Makefile.dep
. The additional 128 bytes deserves some explanation so the user can change it if that is reasonable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rephrased and added the comment in dfdbb86