Server-to-server authentication #1687
Conversation
pcapriotti
changed the base branch from
develop
to
pcapriotti/client-certificates
pcapriotti
force-pushed
the
pcapriotti/s2s-authentication
branch
from
5f27c3e
to
da706ca
Compare
| exampleCert <- liftIO $ BS.readFile "test/resources/unit/localhost.pem" | ||
| let cfg = | ||
| (grpcClientConfigSimple (Text.unpack fedHost) (fromIntegral fedPort) False) | ||
| { _grpcClientConfigHeaders = [("X-SSL-Certificate", exampleCert)] |
There was a problem hiding this comment.
It crossed my mind, but I am not completely sure about this: should the headers list be expanded here rather than replaced?
There was a problem hiding this comment.
The default is empty, so it makes no difference here.
| mockDiscoveryTrivial :: Sem (DiscoverFederator ': r) x -> Sem r x | ||
| mockDiscoveryTrivial = Polysemy.interpret $ \(DiscoverFederator dom) -> | ||
| pure . Right $ SrvTarget (Text.encodeUtf8 (domainText dom)) 443 |
There was a problem hiding this comment.
Given that this function is copy-pasted in ExternalServer.hs, how about moving it to a Util or Interpret module?
There was a problem hiding this comment.
I've now imported it from the other module directly.
akshaymankar
force-pushed
the
pcapriotti/s2s-authentication
branch
from
da706ca
to
39feecd
Compare
pcapriotti
force-pushed
the
pcapriotti/s2s-authentication
branch
3 times, most recently
from
8b6c767
to
5e4819b
Compare
pcapriotti
force-pushed
the
pcapriotti/client-certificates
branch
from
d0f7f06
to
9b56467
Compare
pcapriotti
force-pushed
the
pcapriotti/s2s-authentication
branch
from
a67ab9a
to
084453d
Compare
pcapriotti
force-pushed
the
pcapriotti/client-certificates
branch
from
78bdb9b
to
24aa191
Compare
pcapriotti
force-pushed
the
pcapriotti/s2s-authentication
branch
from
084453d
to
5f83da7
Compare
This will be used for federator integration tests
This adds the same configuration option used for the demo nginx to the nginx ingress.
This is used to set the originDomain field in federated requests within federator integration tests. It cannot be set to a fixed canned value like "example.com", because federators make SRV requests to perform server-to-server authentication, so the domain must be something whose DNS server contains an appropriate SRV record, and so it needs to be set differently according to whether the test is running in the local "demo" environment (where we have a tiny DNS server for "example.com"), or in the CI integration setup, where we can rely on kubernetes DNS server for the federation ingress host.
akshaymankar
force-pushed
the
pcapriotti/s2s-authentication
branch
from
42e7cb7
to
2f6b137
Compare
There was a problem hiding this comment.
This PR seems to make use of a new coreDNS pod, at least there's new config for that used in Run.hs; that's cool to be able to integration test DNS issues.
How do these integration tests work on kubernetes though? Also, I see lots of unit tests but no new integration tests. So where is the coreDNS pod actually used? It would be useful to add some comments in the integration tests that make use of DNS which provide some explanation how DNS is used during discovery for A) local integration tests and B) kubernetes integration tests (I assume some implict DNS names are created by some k8s service that match the name in some config file somewhere in the tests?); as issues arising here at some point in the future may not be easy to figure out.
As for documentation: some guidance how to configure federator and nginx-ingress helm charts for operators so that client certificates are made use of (do we allow server2server authentication to actually be turned off? I suppose we should not?) would be nice (in this or a follow-up PR), and some guidance on how to include your friendly backend's CA in your trust chain would be nice.
As for tests, can we test with a few more scenarios of certificates? E.g. expired, certificate chains of more than one,
| case res of | ||
| Left (InwardError IAuthenticationFailed _) -> pure () | ||
| x -> assertFailure $ "expected IAuthenticationFailed error, got " <> show x |
There was a problem hiding this comment.
maybe create a little helper function similar to expectErr in this file to not duplicate these three lines of case statements for expected errors?
There was a problem hiding this comment.
I just replaced this with asserting more precisely on the errors.
Co-authored-by: jschaul <jschaul@users.noreply.github.com>
With this PR, all the federated requests (even those which bypass nginx, i.e. federator integration tests) need the DNS entry for the origin domain to work. This is not a problem in kubernetes because, as you guessed, there is always an SRV record for the currently deployed federator, so we can just use that to make a request. For local development, however, we don't get this SRV record, so we first considered creating a global record, but decided it was ugly and brittle. So, we chose to just install coredns in our docker-ephmeral setup to facilitate this. The DNS name used by the integration tests is configured in We didn't need more integration tests because all integration tests started needing to provide this cert and all the failure cases were tested in unit test. But I think it would be nice to add one integration test where federation is denied due to wrong certificate would be good.
Yes, we could certainly do a better job at documenting this. Let's take it up right after we merge this.
We don't check expiry or even the chains. This is done by nginx. We could add integration tests, but I think it is ok to just trust nginx here. |
Also, make regeneration optional
This is the final bit of logic implementing server-to-server authentication, namely validation of the domain name provided as part of the request against the certificate checked at the point of SSL termination.
Checklist
make git-add-cassandra-schemato update the cassandra schema documentation.