-
Notifications
You must be signed in to change notification settings - Fork 21
httpd configuration #293
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
httpd configuration #293
Conversation
3bf4fa8 to
0bb2883
Compare
11462a3 to
e581de1
Compare
e581de1 to
c398e74
Compare
|
@ekohl Removed the unnecessary SSL options. One thing I noticed is that the Rails app does not enforce HTTPS. With current changes I can browse and make API calls via |
b08db75 to
5cced9c
Compare
|
I think it'd be good to add a test that port 80 works as expected, e.g.
|
5cced9c to
df390ba
Compare
|
Updated & added tests, the |
df390ba to
8ecad90
Compare
|
For comparison, here's a not sure we need the whole assets blurb at the end -- we don't serve the UI via http-only, but thought it's nice to compare |
I think it does. Prior to this PR it was secure over HTTPS, without it we would regress. |
|
#316 was merged, so you can include |
965d544 to
ec009f7
Compare
ec009f7 to
6e28ce9
Compare
6e28ce9 to
e941490
Compare
e941490 to
13a16c3
Compare
evgeni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline comments.
Also, can you add http versions of test_pub_ca_certificate_downloadable and test_pub_directory_accessible?
* SSL & non-SSL configs for httpd * Rails require_ssl: true * Updated tests
0a1da18 to
55d2ea1
Compare
|
Rebased, updated tests & added new: http/https test_pub_ca_certificate_downloadable & test_pub_directory_accessible |
evgeni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two tiny comments on the tests, but these are cosmetic
Co-authored-by: Evgeni Golov <evgeni@golov.de>
shubhamsg199
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, Only issue is the assets not being served for eg: https://foreman.example.com/pulp/api/v3 , which is expected for now and a low priority issue as discussed on slack thread
We didn't serve them before this PR either so it's not a regression. |
| "ldap-refresh_usergroups", | ||
| ] | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Python convention is to have 2 empty lines here and my linter is unhappy about this.
| @pytest.fixture(scope="module") | ||
| def foreman_status_curl(server): | ||
| return server.run(f"curl --silent --write-out '%{{stderr}}%{{http_code}}' http://{FOREMAN_HOST}:{FOREMAN_PORT}/api/v2/ping") | ||
| return server.run(f"curl --header 'X-FORWARDED-PROTO: https' --silent --write-out '%{{stderr}}%{{http_code}}' http://{FOREMAN_HOST}:{FOREMAN_PORT}/api/v2/ping") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we wanted the test not to rely on Apache :)
|
|
||
| def test_pub_ca_certificate_downloadable(server, certificates, server_fqdn): | ||
| def test_http_pub_ca_certificate_downloadable(server, server_fqdn): | ||
| cmd = server.run(f"curl --silent --output /dev/null --write-out '%{{http_code}}' http://{server_fqdn}/pub/katello-server-ca.crt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| cmd = server.run(f"curl --silent --output /dev/null --write-out '%{{http_code}}' http://{server_fqdn}/pub/katello-server-ca.crt") | |
| cmd = server.run(f"{CURL_CMD} --write-out '%{{http_code}}' http://{server_fqdn}/pub/katello-server-ca.crt") |
|
dang, I merged w/o seeing @ekohl's last comments. feel free to open a follow up |

No description provided.