-
Notifications
You must be signed in to change notification settings - Fork 133
Don't log failure decryption [T2439, T2443] #487
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.
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
Don't log failure decryption [T2439, T2443] #487
Conversation
remove legacy error value use passed logger with context data
…only in debug mode
…and 1.16+ due to changed error type and not working errors.Is for old variant
and re-use it in all other to reduce time of tests
declare correct path to binaries in integration tests
vixentael
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see my comments
| "client_id": string(accessContext.GetClientID()), | ||
| "zone_id": string(accessContext.GetZoneID()), | ||
| }). | ||
| Debugln("Probably error occurred because: 1. used not appropriate TLS certificate or acra-server configured with inappropriate --client_id=<client_id>; 2. forgot to generate keys for your TLS certificate (or with specified client_id); 3. incorrectly configured keystore: incorrect path to folder or Redis database's number") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Debugln("Probably error occurred because: 1. used not appropriate TLS certificate or acra-server configured with inappropriate --client_id=<client_id>; 2. forgot to generate keys for your TLS certificate (or with specified client_id); 3. incorrectly configured keystore: incorrect path to folder or Redis database's number") | |
| Debugln("The error occurred due to one of the following reasons: 1. The client_id from TLS certificate doesn't match the encryption key: check that you are using the appropriate TLS certificate or configure acra-server with a different --client_id=<client_id>. 2. The encryption key for the client_id from TLS certificate is missing, generate encryption keys using keymaker utility for your TLS certificate (or with specified client_id); 3. The required keys are missing in the `keys_dir`, ensure that `keys_dir` param is pointed to a folder with keys or to the correct Redis database's number") |
Take a look @Lagovas , I tried to make messages more friendly and precise
| "client_id": string(accessContext.GetClientID()), | ||
| "zone_id": string(accessContext.GetZoneID()), | ||
| }). | ||
| Debugln("Probably error occurred because: 1. used not appropriate TLS certificate or acra-server configured with inappropriate --client_id=<client_id>; 2. forgot to generate keys for your TLS certificate (or with specified client_id); 3. incorrectly configured keystore: incorrect path to folder or Redis database's number") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
|
updated messages discussed with @vixentael in chat and she has approved |
When I looked how our component that logs confusing message about failure of decryption, I found some legacy unused code (
ErrKeyNotExist,AcraBlock Data Processor), some confusing logs withWarning/Erorrlevels that should useDebug.To avoid logging in deepest level I wrapped error with
fmt.Errorf("%w")to save context, avoid logging and handling it as previous on higher level. And found that in go1.15 previous approach of check thatFileNotExistsuggest to use new package that works okay with wrapped error, but go1.15 doesn't contain this package and needs manual unwrapping before check. So I moved function that uses this API into separate fileserror_check.goanderror_check_go15.gothat included/excluded to compilation with build tags. After removing go1.15 support we need just deleteerror_check_go15.gofile.Additionaly I speeded up our integration tests [T2443] (because I already checked it before and it was fast) by forking ocsp/crl servers on module level. For me it decreases time of tests from 2m+ to 1m+. And made possible to avoid re-compilation binaries for each test run and re-use already compiled binaries that takes a lot of time (especially for tests that generate new configs). And updated CircleCI to build binaries before run any workflow that depends on them (also decreased tests' time on CI to 1-2m).
@vixentael , what do you think about debug log on decryption failure with explanation:
Probably error occurred because: 1. used not appropriate TLS certificate or acra-server configured with inappropriate --client_id=<client_id>; 2. forgot to generate keys for your TLS certificate (or with specified client_id); 3. incorrectly configured keystore: incorrect path to folder or Redis database's numberin
crypto/acrablock.go/crypto/acrastruct.go?Note: it's impossible for now log it as multiline log and one confusing point that it logs before logging with Error level (because it logged on higher level of stack). But it is easy to match that they are related by filepath that wasn't found and mentioned in both log messages.
Checklist
with new changes