jcmturner / gokrb5

Pure Go Kerberos library for clients and services
Apache License 2.0
723 stars 245 forks source link

Issues with OSX ccache v2 cause binary data to be emitted as error message #464

Open dekimsey opened 2 years ago

dekimsey commented 2 years ago

I apologize for the quality of the information here but I hope the report helps, I'm digging pretty deep in my stack to get the root cause of my issue out. I think there are several issues at play and I've tried to identify them below.

Background:

I am attempting to use the terraform DNS provider to make DNS changes. It uses this library to perform some of that work. It's GRPC plugin handling is breaking because this library is emitting binary output in the error struct which it is then propagating up to the terraform client itself

As a user I am seeing:

$ terraform apply -auto-approve
│ Error: Plugin error
│ 
│   with dns_cname_record.endpoint-protection,
│   on main.tf line 1, in resource "dns_cname_record" "foobar":
│    1: resource "dns_cname_record" "foobar" {
│ 
│ The plugin returned an unexpected error from plugin.(*GRPCProvider).ApplyResourceChange: rpc error: code = Internal desc = grpc: error while marshaling: string field contains invalid UTF-8

Issue:

Recompiling terraform-provider-dns to attach a debugger and walking through it, I found keytab/keytab.go emits the raw binary keytab data as part of the error handling.

image

I believe the error handling here is in error. Emitting binary data as part of the error struct is likely to never be a good thing.

Changing the format string to use %q instead addresses the issue, error is propagated successfully up the stack.

│ Error: Error updating DNS record: Error negotiating GSS context: "\x05\x02\x01\x00\x00\x00\x01\x00\x00\x00\r\x00\x00\x00EXAMPLE.COM..."'s length is less than 16777222

I went a bit further and change the error message entirely in hopes of providing some context for resolving Issue 2 below.

//return fmt.Errorf("%q's length is less than %d", b, n+int(l))
return fmt.Errorf("unable to read keytab at position %d, expected %d bytes, but only have %d", n, n+int(l), len(b))

Emits:

│ Error: Error updating DNS record: Error negotiating GSS context: unable to read keytab at position 6, expected 16777222 bytes, but only have 1718

Resolution

Root cause of this behavior was user error. I was passing the CCACHE file as the keytab file path. Those aren't the same files at all silly. I believe the error handling aspect of this is still valid however.

dekimsey commented 2 years ago

I tried a v1 ticket and had more parsing errors. I don't know much about parsing keytabs or why this would fail. But the error handling had the same problem of using %s instead of %v or %q. Specifically the functions readInt16, readInt8, readBytes.