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

ci(nix): Startup/configure apache for renegotiate test under nix #4592

Merged
merged 29 commits into from
Aug 15, 2024

Conversation

dougch
Copy link
Contributor

@dougch dougch commented Jun 10, 2024

Resolved issues:

Partial for #3841

Description of changes:

This PR adds an Apache configuration and startup for the nix devShell, so the integration test renegotiate_apache will pass.

Call-outs:

Apache modules and configuration are somewhat distribution specific, in our current CI setup.

Instead of trying to patch/alter a distribution's apache configs, I've cat'ed together the minimum setup to get the renegotiate_apache test to pass, with as few configuration files as possible.

Testing:

[nix awslc] dougch:~/gitrepos/s2n-tls$ apache2_start
[nix awslc] dougch:~/gitrepos/s2n-tls$ curl -Sk  https://localhost:7777/
<html>
<head>
    <title>Renegotiation Testing Server</title>
</head>
<body>
<p>Welcome to the s2n renegotiation testing server! See the following endpoints:</p>

<ul>
    <li>
        <a href="/change_cipher_suite">/change_cipher_suite</a> forces a renegotiation by changing the negotiated
        cipher suite to AES-128-SHA.
    </li>
    <li>
        <a href="/mutual_auth">/mutual_auth</a> forces a renegotiation by enforcing mutual authentication.
    </li>
</ul>
</body>
</html>

Adhoc codebuild job of just the renegotiate_apache test.

How is this change tested (unit tests, fuzz tests, etc.)? locally, CI

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Jun 10, 2024
@dougch dougch changed the title Nix apache2 ci(nix): Startup/configure apache for renegotiate test under nix Jun 10, 2024
@dougch dougch requested a review from goatgoose June 11, 2024 00:20
@dougch dougch marked this pull request as ready for review June 13, 2024 20:56
Copy link
Contributor

@goatgoose goatgoose left a comment

Choose a reason for hiding this comment

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

If apache configuration is distribution specific, and can't be run with nix without a lot of assumptions, what is the benefit of running it with nix? Would another option be to just run apache in our ubuntu18 CI before running the renegotiate test with nix? If apache can only be run with nix on ubuntu18 anyway, I don't see much of a difference.

nix/shell.sh Outdated Show resolved Hide resolved
nix/shell.sh Outdated Show resolved Hide resolved
nix/shell.sh Outdated Show resolved Hide resolved
nix/shell.sh Outdated Show resolved Hide resolved
codebuild/bin/apache2/apache2.conf Outdated Show resolved Hide resolved
codebuild/bin/install_apache2.sh Outdated Show resolved Hide resolved
codebuild/bin/apache2/apache2.conf Outdated Show resolved Hide resolved
nix/shell.sh Show resolved Hide resolved
tests/integrationv2/apache2/nix/apache2.conf Outdated Show resolved Hide resolved
dougch and others added 3 commits June 17, 2024 09:22
Co-authored-by: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Co-authored-by: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Co-authored-by: Sam Clark <3758302+goatgoose@users.noreply.github.com>
@goatgoose
Copy link
Contributor

If apache configuration is distribution specific, and can't be run with nix without a lot of assumptions, what is the benefit of running it with nix? Would another option be to just run apache in our ubuntu18 CI before running the renegotiate test with nix? If apache can only be run with nix on ubuntu18 anyway, I don't see much of a difference.

We discussed this offline. I was confused - the ubuntu18 configuration was used, but this should work with other environments using nix as well.

@dougch dougch requested a review from goatgoose June 17, 2024 21:46
nix/shell.sh Outdated Show resolved Hide resolved
tests/integrationv2/apache2/nix/conf-enabled/charset.conf Outdated Show resolved Hide resolved
tests/integrationv2/apache2/nix/lock/.gitignore Outdated Show resolved Hide resolved
@dougch dougch requested a review from goatgoose June 18, 2024 23:04
nix/shell.sh Outdated Show resolved Hide resolved
Co-authored-by: Sam Clark <3758302+goatgoose@users.noreply.github.com>
@dougch dougch requested a review from lrstewart June 19, 2024 19:15
@dougch dougch requested a review from goatgoose July 29, 2024 23:12
nix/shell.sh Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary? Wouldn't we just want the defaults?

tests/integrationv2/apache2/nix/conf/apache2.conf Outdated Show resolved Hide resolved
nix/shell.sh Show resolved Hide resolved
Co-authored-by: Lindsay Stewart <stewart.r.lindsay@gmail.com>
@dougch dougch requested a review from lrstewart August 1, 2024 20:54
Comment on lines +229 to +230
SSLCipherSuite HIGH:!aNULL
SSLProtocol all -SSLv3
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this configuration needed? How does it interact with the site-specific configuration?

SSLProtocol -ALL +TLSv1.2
SSLHonorCipherOrder On
SSLCipherSuite HIGH:!aNULL:!MD5
SSLCompression Off
SSLInsecureRenegotiation Off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should only apply to the *:443/ default site

Comment on lines 86 to 89
AddCharset ISO-8859-5 .iso8859-5 .cyr .iso-ru
AddCharset ISO-8859-6 .iso8859-6 .arb .arabic
AddCharset ISO-8859-7 .iso8859-7 .grk .greek
AddCharset ISO-8859-8 .iso8859-8 .heb .hebrew
Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea what most of these character sets are, but since we now only support english (and spanish?), why include the obviously non-english ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trimmed it down some more.

Comment on lines 77 to 80
AddLanguage en .en
# es is ecmascript in /etc/mime.types
RemoveType es
AddLanguage es .es
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does our integ test server need spanish :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't - removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

#4592 (comment)

Unless this has to match some default, can we at least remove the commented out lines? This is almost 2k lines of config I don't think we understand or have really thought about. The most exotic type our integ test server needs is probably json...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trimmed the comments from mime.types

@dougch dougch requested a review from lrstewart August 14, 2024 17:36
Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

I still think we can get the apache config more focused and minimal (especially mime.types), but this is fine.

@dougch dougch enabled auto-merge (squash) August 14, 2024 20:52
@dougch dougch merged commit 45bf1d4 into aws:main Aug 15, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants