Creating only one IAuthenticatedEncryptor per IKey #221
Conversation
| } | ||
| } | ||
|
|
||
| return _encryptor; |
There was a problem hiding this comment.
Does this need to be thread safe?
There was a problem hiding this comment.
Yes and we already do a lock here https://github.com/aspnet/DataProtection/pull/221/files#diff-3dda400bcdb5a80421cdd26389311c63R77. The other places that we call this are just for Debug.Assert and we don't use those objects that are created. This has been the same since before the refactor.
There was a problem hiding this comment.
You still haven't really answered this. If the invariant that we only create one is important, this code isn't correct.
There was a problem hiding this comment.
Spoke offline. This lock here was added as a perf optimization and we don't need to make changes to this.
| /// to and decrypt data from this key. | ||
| /// </summary> | ||
| /// <returns>An <see cref="IAuthenticatedEncryptor"/>.</returns> | ||
| IAuthenticatedEncryptor CreateEncryptorInstance(); |
There was a problem hiding this comment.
This should be called CreateEncryptor
|
I'm not sure I understand the problem that's being solved here. This change basically reverts the design, now instead of being data the key is a factory |
|
Spoke offline. We need |
ajaybhargavb
force-pushed
the
ajbaaska/fix-encryptor
branch
from
702e465 to
e55da74
Compare
|
🆙 📅 No longer in design |
ajaybhargavb
changed the title
ajaybhargavb
force-pushed
the
ajbaaska/fix-encryptor
branch
from
e55da74 to
c959795
Compare
Issue - #220
Problem:
We should be creating only one
IAuthenticatedEncryptorinstance per key. After #134, we now have multiple instances being created.Solution:
Brought back
CreateEncryptorInstance()onIKeyWhy was this needed?
Currently
IKeyandIAuthenticatedEncryptorcreation are decoupled. Because of that there was no way to associate an encryptor instance with a key. HavingCreateEncryptorInstance()onIKeylets us use the same encryptor instance across multiple calls.@rynowak