NO-ISSUE - Add CA retrieve option#24
Conversation
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
dborovcanin
left a comment
There was a problem hiding this comment.
How is viewing CA different from any other certificate?
| var err error | ||
| switch len(serialNumber) { | ||
| case 0: | ||
| jwtToken := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.StandardClaims{ExpiresAt: time.Now().Add(time.Minute * 5).Unix(), Issuer: Organization, Subject: "certs"}) |
There was a problem hiding this comment.
Extract 5 minutes to a constant.
| token, err := jwtToken.SignedString([]byte(serialNumber)) | ||
| if err != nil { | ||
| return "", errors.Wrap(ErrGetToken, err) | ||
| func (s *service) RetrieveCertDownloadToken(ctx context.Context, serialNumber ...string) (string, error) { |
There was a problem hiding this comment.
This method signature is cryptic. First, serialNumber should be plural. Second, what does it mean to have multiple serial numbers?
There was a problem hiding this comment.
Am trying to reusing the same this function to get both a client and a CA. I cater for only 2 case if there is one serial number and none. You can pass more that one since it will default to an illegal length of serial numbers.
There was a problem hiding this comment.
I can create a separate method for handling CAs
There was a problem hiding this comment.
You can have only one serial number. If it's empty - fetch CA. You can map it to two endpoint methods to separate cases where an empty serial number is a mistake and when it's not.
|
|
||
| // RetrieveCertDownloadToken retrieves a certificate download token. | ||
| RetrieveCertDownloadToken(ctx context.Context, serialNumber string) (string, error) | ||
| RetrieveCertDownloadToken(ctx context.Context, serialNumber ...string) (string, error) |
There was a problem hiding this comment.
Explain - what's this method even used for?
| if err != nil { | ||
| return Certificate{}, errors.Wrap(ErrViewEntity, err) | ||
| func (s *service) ViewCert(ctx context.Context, serialNumber ...string) (Certificate, error) { | ||
| var cert Certificate |
There was a problem hiding this comment.
Same, this becomes cryptic if we have multiple serial numbers. Especially given we use slice exclusively, i.e. we either use intermediate CA, or, for whatever reason, only the first element of the slice.
Normally they are generated during initialization and there is no implemented method to retrieve or view them. |
dborovcanin
left a comment
There was a problem hiding this comment.
@nyagamunene Please fix the previous remarks related to the serial number slice parameter.
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
What type of PR is this?
This is a feature because it adds the following functionality: To retrieve the CA.
What does this do?
It adds an option to retrieve the signing CA i.e. intermediate CA. This is currently needed for mutual TLS.
Which issue(s) does this PR fix/relate to?
N/A
Have you included tests for your changes?
Yes.
Did you document any new/modified features?
No.
Notes