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

Add bindings for DANE #14

Merged
merged 1 commit into from
Aug 31, 2024
Merged

Add bindings for DANE #14

merged 1 commit into from
Aug 31, 2024

Conversation

coma64
Copy link

@coma64 coma64 commented Aug 27, 2024

Overview

First of all, thanks for keeping this library maintained :D

This PR adds CGO bindings for a bunch of openssl function, most notably DANE (DNS-based Authentication of Named Entities) related functions.

New features

  • Bindings for DANE
  • Bindings for TLS handshake tracing
  • Bindings for X509_digest()
  • Bindings for X509_verify_cert_error_string()
  • Bindings for SSL_get_version()

@oleg-jukovec oleg-jukovec requested a review from DerekBum August 27, 2024 19:16
@oleg-jukovec
Copy link
Collaborator

Thank you for the patch! Please, fix the CI errors:

Error: ./ssl.go:219:12: cannot use _Ctype_ulong(len(data)) (value of type _Ctype_ulong) as type _Ctype_uint in variable declaration
FAIL	github.com/tarantool/go-openssl [build failed]
?   	github.com/tarantool/go-openssl/utils	[no test files]
FAIL
Error: Process completed with exit code 2.

before a review.

@coma64
Copy link
Author

coma64 commented Aug 28, 2024

@oleg-jukovec Sorry for the inconvenience, I didn't enable actions on the fork... Everything should be fixed now

Copy link

@DerekBum DerekBum left a comment

Choose a reason for hiding this comment

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

Thank you for the patch! Overall you did a great work, docs are clear and codestyle is on the level. That was a pleasure to review this patch.

But there are still some moments I would like you to fix or discuss with us:

  1. You should squish your commits into one (or more, but logically separated). Right now its a mess, some commits are breaking the CI, some revert changes from other commits.
    Please, also write a commit message body (if needed) to explain the changes done for each commit. You can refer to the commit history the master branch.
  2. In Tarantool every commit should end with a dot. Thank you for following this rule for code comments, but there are some missing dots in CHANGELOG and README.
  3. There are some other nits and questions described below. Please forgive me in case of being too pedantic and don't be afraid to engage in dialogue with me. I hope all the comments are clear.

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ssl.go Outdated Show resolved Hide resolved
ssl.go Outdated Show resolved Hide resolved
ctx.go Outdated Show resolved Hide resolved
ctx.go Outdated Show resolved Hide resolved
ssl.go Outdated Show resolved Hide resolved
ssl.go Outdated Show resolved Hide resolved
ssl.go Outdated Show resolved Hide resolved
ssl.go Outdated Show resolved Hide resolved
@DerekBum
Copy link

Another thing I forgot about is the lack of tests. Is there any difficulties writing unit tests for new methods?

@coma64 coma64 force-pushed the master branch 2 times, most recently from 0f86b1e to a875f95 Compare August 30, 2024 09:11
@coma64
Copy link
Author

coma64 commented Aug 30, 2024

No, for our applications this was covered by e2e tests but that's of course not the case for this library. I'll write some

@coma64
Copy link
Author

coma64 commented Aug 30, 2024

Everything should be resolved now. Please have a look and I'll squash the commits @DerekBum

Copy link

@DerekBum DerekBum left a comment

Choose a reason for hiding this comment

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

Thank you for fixing all the countless nits I had! Everything looks great, CI is all green. Just a couple of comment fixes left (just them, everything else is ok).

I will approve this PR after the squash.

ssl_test.go Outdated Show resolved Hide resolved
ssl.go Outdated Show resolved Hide resolved
ssl.go Outdated Show resolved Hide resolved
@coma64
Copy link
Author

coma64 commented Aug 30, 2024

Everything's squashed now, thanks for the quick review! :)

Copy link

@DerekBum DerekBum left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the patch!

@oleg-jukovec oleg-jukovec merged commit 39dabe3 into tarantool:master Aug 31, 2024
9 checks passed
@oleg-jukovec
Copy link
Collaborator

@coma64 do you need a release version with the patchset?

@coma64
Copy link
Author

coma64 commented Aug 31, 2024

Yes please

oleg-jukovec added a commit that referenced this pull request Aug 31, 2024
The release adds more bindings.

Added

- Bindings for DANE (#14).
- Bindings for TLS handshake tracing (#14).
- Bindings for `X509_digest()` (#14).
- Bindings for `X509_verify_cert_error_string()` (#14).
- Bindings for `SSL_get_version()` (#14).
@oleg-jukovec oleg-jukovec mentioned this pull request Aug 31, 2024
oleg-jukovec added a commit that referenced this pull request Aug 31, 2024
The release adds more bindings.

Added

- Bindings for DANE (#14).
- Bindings for TLS handshake tracing (#14).
- Bindings for `X509_digest()` (#14).
- Bindings for `X509_verify_cert_error_string()` (#14).
- Bindings for `SSL_get_version()` (#14).
oleg-jukovec added a commit that referenced this pull request Aug 31, 2024
The release adds more bindings.

Added

- Bindings for DANE (#14).
- Bindings for TLS handshake tracing (#14).
- Bindings for `X509_digest()` (#14).
- Bindings for `X509_verify_cert_error_string()` (#14).
- Bindings for `SSL_get_version()` (#14).
@oleg-jukovec
Copy link
Collaborator

Yes please

https://github.com/tarantool/go-openssl/releases/tag/v1.1.0

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