rbd: add kmip encryption type #3306
Conversation
|
@Rakshith-R looks like |
|
Please split the vendor part from the actual feature, that makes it much easier to review. |
|
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
Rakshith-R
force-pushed
the
kmip
branch
2 times, most recently
from
135c05d to
7ae7bd7
Compare
Rakshith-R
force-pushed
the
kmip
branch
2 times, most recently
from
80721b4 to
8b69305
Compare
|
@Rakshith-R can you please address the comments.. I am planning to start the Release work of 3.7, thats why :) |
Rakshith-R
force-pushed
the
kmip
branch
2 times, most recently
from
3463ef6 to
345307c
Compare
Rakshith-R
requested review from
humblec and
SanjalKatiyar
and removed request for
SanjalKatiyar
internal/kms/kmip.go
Outdated
| return conn, nil | ||
| } | ||
|
|
||
| // discover performs KMIP handshake. |
There was a problem hiding this comment.
It would be useful to link to the documentation (+chapter/paragraph) where this handshake is explained. Maybe for other protocol procedures too.
There was a problem hiding this comment.
Its just discover protocol being run, linked discover operation chapter here.
| } | ||
|
|
||
| // KMIP handshake | ||
| err = kms.discover(conn) |
There was a problem hiding this comment.
just above is conn.Handshake()... what's the difference?
There was a problem hiding this comment.
removed the term handshake, added more explanation in function description.
internal/kms/kmip.go
Outdated
| func (kms *kmipKMS) connect() (*tls.Conn, error) { | ||
| conn, err := tls.Dial("tcp", kms.endpoint, kms.tlsConfig) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
ideally add some more context in the returned errors with fmt.Errorf(...)
| }, | ||
| }) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
make sure that returned errors are unique, so that tracking the source of the error trivial. Use fmt.Errorf(...) for returned errors if you can.
internal/kms/kmip.go
Outdated
|
|
||
| req, err := ttlv.Marshal(msg) | ||
| if err != nil { | ||
| return nil, nil, nil, err |
There was a problem hiding this comment.
same here, use fmt.Errorf() so that tracking the source of errors becomes easy
internal/kms/kmip.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // send sends KMIP operation over tls connection, return response and error. |
There was a problem hiding this comment.
return response and error... but there are 4 return values? probably good to explain each of them
This was tested with pykimp server instance deployed in kubernetes cluster, |
The Key Management Interoperability Protocol (KMIP) is an extensible communication protocol that defines message formats for the manipulation of cryptographic keys on a key management server. Ceph-CSI can now be configured to connect to various KMS using KMIP for encrypting RBD volumes. https://en.wikipedia.org/wiki/Key_Management_Interoperability_Protocol Signed-off-by: Rakshith R <rar@redhat.com>
Signed-off-by: Rakshith R <rar@redhat.com>
The Key Management Interoperability Protocol (KMIP)
is an extensible communication protocol
that defines message formats for the manipulation
of cryptographic keys on a key management server.
Ceph-CSI can now be configured to connect to
various KMS using KMIP for encrypting RBD volumes.
https://en.wikipedia.org/wiki/Key_Management_Interoperability_Protocol
Resolves: #3282
Signed-off-by: Rakshith R rar@redhat.com
Heavily inspired from noobaa/noobaa-operator#964 (comment)