feat(admin-api) support for SSL in the admin API #1706
Conversation
|
|
||
| if not pl_path.exists(ssl_cert) and not pl_path.exists(ssl_cert_key) then | ||
| log.verbose("generating default SSL certificate and key") | ||
| log.verbose("generating "..(admin and "admin" or "default").." SSL certificate and key") |
There was a problem hiding this comment.
Let's make this more concise:
log.verbose("generating %s SSL certificate and key",
admin and "admin" or "default") -- notice the line jump to avoid >80 cols| end | ||
| end | ||
| else | ||
| log.verbose("default SSL certificate found at %s", ssl_cert) | ||
| log.verbose((admin and "admin" or "default").." SSL certificate found at %s", ssl_cert) |
| @@ -8,6 +8,9 @@ cluster_listen_rpc = 127.0.0.1:9373 | |||
| ssl_cert = spec/fixtures/kong_spec.crt | |||
| ssl_cert_key = spec/fixtures/kong_spec.key | |||
|
|
|||
| admin_ssl_cert = spec/fixtures/kong_spec.crt | |||
| admin_ssl_cert_key = spec/fixtures/kong_spec.key | |||
There was a problem hiding this comment.
I think we should disable admin_ssl (and ssl altogether) from being used in our tests. We do not have any test even making use of them, and if that is or becomes the case, we can enable SSL for this particular test when starting kong in its setup() hook.
We should keep the test instance as lean as possible.
There was a problem hiding this comment.
This test is using them https://github.com/Mashape/kong/pull/1706/files#diff-7c3103b7930d1a3b80a29ec77ef516cd
There was a problem hiding this comment.
Then as I said, in the context of this test and this test only should it be enabled.
There was a problem hiding this comment.
I am testing it the same way we are testing the HTTPs proxy port. These things should be tested.
There was a problem hiding this comment.
? I am not saying they shouldn't be tested, I am saying to only enable SSL in the related tests.
There was a problem hiding this comment.
Why complicating our lives in tests? If anything, the admin API should only be available on SSL since it's being used to provision security credentials.
thibaultcha
added
pr/changes requested
subnetmarco
removed
the
pr/changes requested
thibaultcha
added
the
pr/changes requested
Serving the Admin API over HTTPs as a first step to implement first-class authentication in the Admin API (most likely Basic Authentication) without the ugly hack of adding the Admin API as a regular API on Kong.
Full changelog