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

Dtls #18

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Dtls #18

wants to merge 7 commits into from

Conversation

seojeongmoon
Copy link
Collaborator

I changed the code to encrypt the messages, but my version saul_coap is behind yours.

Copy link
Collaborator

@rosetree rosetree left a comment

Choose a reason for hiding this comment

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

Thanks for your submit

url = [email protected]:seojeongmoon/RIOT.git
branch = dtls
Copy link
Collaborator

Choose a reason for hiding this comment

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

You verified, that the changes you made in this RIOT fork, are merged into the RIOT upstream; do I remember this correctly? So we wouldn’t need to change this submodule, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, as long as the upstream is updated it is fine.

* @brief tlsman test application (PSK and ECC keys)
*
* Small test for TLSMAN. Many definitions defined here are also available at
* sock_secure (and are intended to be used in standard applications)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is originally meant to be a test. What do we use it for? To define the constants below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the keys are defined.

0x43, 0x21, 0x43, 0x5B, 0x7A, 0x80, 0xE7, 0x14,
0x89, 0x6A, 0x33, 0xBB, 0xAD, 0x72, 0x94, 0xCA,
0x40, 0x14, 0x55, 0xA1, 0x94, 0xA9, 0x49, 0xFA
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I don’t know / understand enough about DTLS in RIOT. But it doesn’t feel secure to hard code a private key in a public repository. Is this meant to be used that way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right. This is an example code. When we use it, we should replace with our own keys. (using special random generator for cryptography)

extern const unsigned char ecdsa_pub_key_y[];
#endif /* DTLS_ECC */
#endif /* MODULE_SOCK_DTLS */

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just some minor code ordering issue: It suggest to move this up a bit. So that all included files are visible in the same spot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that won't be a problem


/* tell gcoap with tag to use */
gcoap_set_credential_tag(SOCK_DTLS_GCOAP_TAG);
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about the usage of this repository, I have some thoughts:

  • We would need a bit of documentation, to explain at least, how DTLS_PSK and DTLS_ECC differ. Maybe pointing to different resources.

  • Securing the network communication would be great. But in our context, I would prioritize compatibility with other groups (the web application or other nodes). Could you find out, what they would need to do, to support DTLS on their side? Maybe point to some documentation, as well.

  • More fundamental question: should we do this in our repository? I imagine, that this is part of a bigger system and is combined with other projects. What if this system combines two coap projects with DTLS configuration? Would this work? Or should the systems main.c handle the encryption on its own?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. I asked the guy(pokgak), he said ECC is too slow on hardware so we should use PSK.

  2. If they use the latest RIOT version that merged the pull request of pokgak, then it wouldn't be a problem. If not, that would be impossible because both communicators must have dtls.

  3. Yes that is a good point. I was thinking about it too. I think we can have two branches, one with DTLS and one without and use one without first. It is good but I would prioritize everything working over security right now :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you create a dtls branch, then I will pull request to that branch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Successfully merging this pull request may close these issues.

3 participants