Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Koperator can handle intermediate and leaf certificates in generated kafkaUser's TLS certificate #843

Merged
merged 4 commits into from
Aug 2, 2022
Merged

Koperator can handle intermediate and leaf certificates in generated kafkaUser's TLS certificate #843

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1bf8374
Fix intermediate cert handling for ssl listeners
bartam1 Aug 1, 2022
1fc54b7
Fix based on review 1
bartam1 Aug 1, 2022
3fb43bc
Refactor 1
bartam1 Aug 2, 2022
aeca334
Merge branch 'master' into fixsslmulticert
bartam1 Aug 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion controllers/kafkauser_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,13 @@ func (r *KafkaUserReconciler) Reconcile(ctx context.Context, request reconcile.R
return requeueWithError(reqLogger, "failed to reconcile user secret", err)
}
}
kafkaUser = user.DN()
kafkaUser, err = user.DN()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: we need to rename that function for public interface clarity purposes....

if err != nil {
reqLogger.Error(err, "could not get Distinguished Name from the generated TLS certificate")
return ctrl.Result{
Requeue: false,
}, err
}
// check if marked for deletion and remove created certs
if k8sutil.IsMarkedForDeletion(instance.ObjectMeta) {
reqLogger.Info("Kafka user is marked for deletion, revoking certificates")
Expand Down
4 changes: 1 addition & 3 deletions pkg/util/cert/certutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,7 @@ func DecodeCertificate(raw []byte) (cert *x509.Certificate, err error) {
if err != nil {
return nil, err
}
if len(certs) != 1 {
return nil, errors.New("only one certificate should be present, more found")
}
panyuenlau marked this conversation as resolved.
Show resolved Hide resolved

return certs[0].Certificate, nil
}

Expand Down
9 changes: 6 additions & 3 deletions pkg/util/pki/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,13 @@ type UserCertificate struct {
}

// DN returns the Distinguished Name of a TLS certificate
func (u *UserCertificate) DN() string {
func (u *UserCertificate) DN() (string, error) {
// cert has already been validated so we can assume no error
cert, _ := certutil.DecodeCertificate(u.Certificate)
return cert.Subject.String()
cert, err := certutil.DecodeCertificate(u.Certificate)
if err != nil {
return "", err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: wouldn't it be better to wrap the error with some content information (we are doing this in KafkaUser DistinguishedName retrieval), IMO it would help with debug clarification.
(These cases I always have the os.Open() errors in mind where the non-existing path object error is no such file or directory, but the path itself is not included - although that would be a detail, not a wrapping, but I mean the contextual information in general).

Copy link
Member

@stoader stoader Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if any useful information is available here that could be added to the error context. The caller can add more details to the error like the KafkaUser custom resources name (https://github.com/banzaicloud/koperator/pull/843/files#diff-2b2cc6c6efb0a3a4b49078c08a64ff936f6d02a73543d0c8f98a7b9b0c4f2a2eR243)

Maybe a more descriptive error message could help here errors.WrapIfWithDetails(err, "couldn't decode KafkaUser certificate", "cert", string(u.Certificate))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I meant an error message to narrow the context from generic certificate error to KafkaUser DistringuishedName determination certificate error

Copy link
Contributor Author

@bartam1 bartam1 Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error log message already contains the kafkauser name, namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reconciler dumps the stacktrace.

}
return cert.Subject.String(), nil
}

// GetInternalDNSNames returns all potential DNS names for a kafka cluster - including brokers
Expand Down
5 changes: 4 additions & 1 deletion pkg/util/pki/pki_common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,10 @@ func TestDN(t *testing.T) {
userCert := &UserCertificate{
Certificate: cert,
}
dn := userCert.DN()
dn, err := userCert.DN()
if err != nil {
t.Errorf("error should be nil, got: %s", err)
}
if dn != expected {
t.Error("Expected:", expected, "got:", dn)
}
Expand Down