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

feat: add support for certificate authentication #12

Merged
merged 4 commits into from
Jul 22, 2023
Merged

Conversation

hasheddan
Copy link
Contributor

Updates xk6-coap with support for certificate authentication.

Signed-off-by: Daniel Mangum georgedanielmangum@gmail.com

Fixes #8

Copy link

@tim-golioth tim-golioth left a comment

Choose a reason for hiding this comment

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

minor comments, but otherwise LGTM. nice work!

README.md Outdated
"",
"",
"path/to/client/crt.pem",
"path/to/client/key.pem",

Choose a reason for hiding this comment

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

looks like the indenting might be off here? or it's just Github rendering being flaky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch, thanks!

README.md Outdated
"COAP_PSK_ID",
"COAP_PSK",
"path/to/client/crt.pem",
"path/to/client/key.pem",

Choose a reason for hiding this comment

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

same here.

coap/coap.go Outdated
}

// Only ECDSA keys are currently supported.
if !goja.IsUndefined(cc.Argument(3)) && !goja.IsUndefined(cc.Argument(4)) {

Choose a reason for hiding this comment

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

i take it cc.Argument(3) and cc.Argument(4) are the PSK and PSK-ID? would be nice if this could be an enum or a const that was more human-readable, e.g. ARG_PSK, ARG_PSK_ID or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call -- I'll move all the arg indices up to const 👍🏻

coap/coap.go Outdated

// Only ECDSA keys are currently supported.
if !goja.IsUndefined(cc.Argument(3)) && !goja.IsUndefined(cc.Argument(4)) {
pemCert, err := os.ReadFile(filepath.Clean(cc.Argument(3).String()))

Choose a reason for hiding this comment

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

might be worth considering getting the absolute path of the file using filepath.Abs(). i tend to use that to avoid errors related to not found errors related to relative paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm what is filepath.Abs() giving us here that filepath.Clean() is not? If provided path is not already absolute it will ultimately just concatenate with the working directory and clean and given that we aren't restricting any file paths here then I don't think that it will actually prevent any errors (i.e. if the relative path supplied is invalid the absolute one will be also).

Updates `xk6-coap` with support for certificate authentication.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
Updates .gitignore to ignore PEM files to avoid accidentally leaking
certs and keys.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
Updates the simple.js example to specify the now required PSK env vars.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
Updates README.md to specify the supported authentication mechanisms.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
@hasheddan hasheddan merged commit 903ebc3 into main Jul 22, 2023
1 check passed
@hasheddan hasheddan deleted the feat/cert-auth branch August 1, 2023 12:58
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.

Add support for certificate authentication
2 participants