uuid endpoint #39
uuid endpoint #39
Conversation
pkg/server/apiHandler.go
Outdated
| http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| cert, err := parseX509Cert(uuidRequest.GetDeviceCert()) |
There was a problem hiding this comment.
Why are we using this new parseX509Cert() func, and not the existing getClientCert() that all of the others use? Is this different somehow?
There was a problem hiding this comment.
In this case, you need to take the certificate from the UuidRequest object, not from the request itself.
There was a problem hiding this comment.
Is that documented somewhere? Is this fundamentally different than all the other requests?
There was a problem hiding this comment.
In the V2 API I'd expect the full client cert to be in the authcontainer. If we also have it in the UuidRequest it means that for V2 we'd have it in two places; either they need to be checked to be identical or we don't use the UuidRequest DeviceCert (latter would make the message a lot smaller.)
If we support the uuid API endpoint in the V1 API it means we do need the DeviceCert in the uuid request.
There was a problem hiding this comment.
So, we need to add check that the TLS cert is the same with DeviceCert inside UuidRequest?
There was a problem hiding this comment.
I am not clear on this. Is /uuid v1 or v2? If it is v1, then the device cert needs to be available via mTLS. If it is v2, we should just defer on adding /uuid until adam goes v2.
There was a problem hiding this comment.
Somewhat related question, but for the other APIs (config, metrics, info) where we have a UUID in the URL and one which we fetch based on the cert, does Adam/Eden check that they match?
There was a problem hiding this comment.
Seems we do not use/check uuid from url (only uuid of apps).
giggsoff
force-pushed
the
uuid-endpoint
branch
from
7ab2a71 to
d8132b5
Compare
pkg/server/server.go
Outdated
| } | ||
|
|
||
| //parseX509Cert parses incoming certificate | ||
| func parseX509Cert(inp []byte) (*x509.Certificate, error) { |
There was a problem hiding this comment.
Either the cert needs to be removed from the uuid message and you get the cert from the authcontainer, or you need to check that the cert in the uuid message and the authcontainer are identical.
There was a problem hiding this comment.
Coming back to the earlier conversation, isn't authcontainer an entirely v2 construct? We should use defer on this until we get adam to v2.
giggsoff
force-pushed
the
uuid-endpoint
branch
from
d8132b5 to
7eef66f
Compare
pkg/server/apiHandler.go
Outdated
| http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) | ||
| return | ||
| } | ||
| if u.String() != uuidTLS.String(){ |
There was a problem hiding this comment.
Added a check for a uuid match for certificates from TLS and from uuidRequest
|
@giggsoff can you please hold off from merging this, since there is a namespace conflict in python generated protofiles (due to uuid namespace collision), for which I need to move uuid/uuid.proto to eveuuid/eveuuid.proto. |
giggsoff
force-pushed
the
uuid-endpoint
branch
from
7eef66f to
192db51
Compare
giggsoff
force-pushed
the
uuid-endpoint
branch
from
192db51 to
01f906f
Compare
giggsoff
force-pushed
the
uuid-endpoint
branch
from
01f906f to
e63accc
Compare
|
So I see this is worked on again. Is the idea to merge it this time @giggsoff ? |
|
Yes, seems we need this PR for lf-edge/eve#2330 (comment) |
Signed-off-by: Petr Fedchenkov <[email protected]>
giggsoff
force-pushed
the
uuid-endpoint
branch
from
e63accc to
2085db5
Compare
Added. I can see some problems with redis tests. Will work on them in another PR. |
Implementation of the /uuid endpoint #38
Signed-off-by: Petr Fedchenkov [email protected]