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

wip boringssl #40

Closed
wants to merge 2 commits into from
Closed

wip boringssl #40

wants to merge 2 commits into from

Conversation

fffonion
Copy link

@fffonion fffonion commented Mar 4, 2022

add an option to start gojira with boringssl build

@fffonion fffonion force-pushed the boringssl branch 8 times, most recently from 5ed4b67 to 4a47b22 Compare March 4, 2022 07:15
Copy link
Contributor

@mikefero mikefero left a comment

Choose a reason for hiding this comment

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

Looks good so far; I understand this is a WIP so providing initial feedback.

gojira.sh Outdated Show resolved Hide resolved
gojira.sh Outdated Show resolved Hide resolved
gojira.sh Outdated Show resolved Hide resolved
gojira.sh Outdated Show resolved Hide resolved
@aboudreault
Copy link
Contributor

@fffonion should we finalize this one? I think boringssl will be ready to be tested soon, am I right?

@fffonion
Copy link
Author

fffonion commented Apr 7, 2022

Hi @aboudreault @mikefero thanks for the review, this is no longer in priority, I managed to run the matrix in travis instead. I will address the comments shortly however.

@aboudreault aboudreault self-assigned this Apr 8, 2022
@@ -552,6 +552,7 @@ function image_name {
OPENRESTY=${OPENRESTY:-$(req_find $req_file RESTY_VERSION)}
LUAROCKS=${LUAROCKS:-$(req_find $req_file RESTY_LUAROCKS_VERSION)}
OPENSSL=${OPENSSL:-$(req_find $req_file RESTY_OPENSSL_VERSION)}
BORINGSSL=${BORINGSSL:-$(req_find $req_file RESTY_BORINGSSL_VERSION)}
Copy link
Contributor

Choose a reason for hiding this comment

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

the var name will probably be modified in kong-ee? https://github.com/Kong/kong-ee/blob/master/.requirements#L13

Copy link
Author

Choose a reason for hiding this comment

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

not sure i get this one, are you suggesting to rename this variable to name BORINGSSL_VERSION that aligns to .requirements?

Copy link
Contributor

Choose a reason for hiding this comment

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

.requirements uses BORINGSSL_VERSION and the gojira PR is trying to read RESTY_BORINGSSL_VERSION. so just asking which one is correct.

@aboudreault
Copy link
Contributor

aboudreault commented Apr 14, 2022

@fffonion I got some issues locally while building. I couldn't fix in your fork so I pushed an up-to-date branch here: https://github.com/Kong/gojira/tree/boringssl

The build succeed using that branch. However, when I try a make dev in my gojira env, I got the following error:

ldoc 1.4.6-2 is now installed in /build/luarocks (license: MIT/X11)

Error: ssl_dhparam: no such file at ffdhe2048
  Run with --v (verbose) or --vv (debug) for more details
Error: ssl_dhparam: no such file at ffdhe2048
  Run with --v (verbose) or --vv (debug) for more details

@aboudreault
Copy link
Contributor

@fffonion do you know what is the ssl_dhparam error?

@fffonion
Copy link
Author

fffonion commented Apr 27, 2022

@aboudreault yes that's because boringssl doesn't implement those pre-defined groups. In fact, we should always set ssl_ciphers=fips for the boringssl build (or even better fips=on as this flags turns on ssl_ciphers=on), those will be default turned on in the released FIPS build in the future. We will need the boringssl-matrix branch on kong-ee.

@aboudreault
Copy link
Contributor

closing in favor of #48

@aboudreault aboudreault closed this May 4, 2022
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