Fixes bug in error handling within the VerifyFileExists method
#256
Conversation
Fixes bug in error handling within the `VerifyFileExists` method that resulted in a panic when the error from `os.Stat` was not `ErrNotExist`. The fix includes introducing the `CAKC058` error and log message for when the path to a file exists but is not a regular file.
There was a problem hiding this comment.
I think we really should add checking for and logging of permissions errors (in addition to logging errors that indicate non-regular files). There are a few reasons why I'm thinking that we're getting hit with permissions errors when checking for the existence of the authenticator client certificate:
-
If you look at the first (top) entry in the crash traceback:
github.com/cyberark/conjur-authn-k8s-client/pkg/utils.VerifyFileExists(0x84dc32, 0x1a, 0x0, 0xc00043e000) /opt/conjur-authn-k8s-client/pkg/utils/file.go:50 +0xb1the first 2 hexadecimal arguments shown on the stack, i.e.
0x84dc32, 0x1a, represent a pointer and a length for thepathstring argument being passed to the function. (The last two represent the return value, which also happened to get added to the stack when Golang gets compiled/run).This says that the length of the string
pathbeing passed is0x1a, or 26 decimal. This is EXACTLY what we're expecting in this case, because we're checking for the authenticator client cert at the path/etc/conjur/ssl/client.pem, which is 26 characters long. So it appears that at least the length of the string is not junk... which leads me to believe that the pointer pointing to the text is probably not junk either.
-
The applications are able to authenticate when version 0.16.1 of the authenticator was used, but fail to authenticate when 0.19.1 is used.
One of the differences between v0.16.1 and v0.19.1 that might affect access to the cert at
/etc/conjur/ssl/client.pemis that the Dockerfile for building the authenticator was changed a bit between these versions:
- For v0.16.1, the/etc/conjur/ssldirectory is created with permissions0777, and that directory is created by userrootin the Docker build process.
- For v0.19.1, the/etc/conjur/ssldirectory is created with permissions0770, and that directory is created by userauthenticator(and the container's user is set toauthenticator).Everything looks like it should be okay and the
/etc/conjur/ssl/client.pemshould be readable in both cases, but it does seem a bit suspicious.I started to add this check in, and it was pretty easy in the functional/production code, but the UT for this is difficult as it's hard to trigger a permissions error in calls to
os.Stat(). So this needs to be mocked. I'm close to having something that might work.
diverdane
force-pushed
the
patch
branch
2 times, most recently
from
c29a8ac to
85c7683
Compare
|
@doodlesbykumbi I've added error logging for a file permissions error (for reasons listed earlier), but unit testing this required a restructuring of the mocking that was done for testing files.go. Let me know what you think... if this looks like a bad idea you can do a force push to remove this change. |
| isRegular: false}, | ||
| nonRegularFilePath: testConfig{ | ||
| useOSUtils: true, | ||
| statError: nil, |
There was a problem hiding this comment.
It might be a little clearer if we skip the statError and isRegular assignments in this particular case since we're not mocking these when useOSUtils is true.
| // Test config for a given file scenario | ||
| type testConfig struct { | ||
| useOSUtils bool | ||
| statError error // This is a don't care if useOSUtils is 'true' |
There was a problem hiding this comment.
I'm equivocating on whether these would be more clearly named mockStatError and mockIsRegular in this struct (rather than statError and isRegular)? Although it's pretty obvious what it's used for, so I'll defer to your choice.
fileutils is an internal detail. This also allows us to maintain the interfaces of WaitForFile and VerifyFileExists
|
@suefran - can you please take this? |
|
I'm on it, thanks! |
What does this PR do?
Fixes bug in error handling within the
VerifyFileExistsmethod that resulted in a panic when the error fromos.Statwas notErrNotExist. The fix includes introducing theCAKC058error and log message for when the path to a file exists but is not a regular file.What ticket does this PR close?
Resolves ##252
Checklists
Change log
Test coverage
Documentation
READMEs) were updated in this PR, and/or there is a follow-on issue to update docs, orManual tests
If you are preparing for a release, have you run the following manual tests to verify existing functionality continues to function as expected?