Eliminate persistence of secrets entirely #522
Conversation
snazy
requested review from
jbonofre,
ashvina,
RussellSpitzer,
vvcephei,
takidau,
jackye1995,
flyrain,
eric-maynard,
collado-mike,
ebyhr and
adutra
as code owners
| } else { | ||
| var principalSecrets = secretsResult.getPrincipalSecrets(); | ||
| System.out.printf( | ||
| "Bootstrap realm '%s' - root principal credentials: %s secret: %s", |
There was a problem hiding this comment.
Do we want to always print this? Users can provide their own secrets for bootstrapping via env. variables.
There was a problem hiding this comment.
Also, IIRC, secrets are not stored in EclipseList anyway and the in-memory persistence prints the generated secrets in InMemoryPolarisMetaStoreManagerFactory (end of file).
I tend to think this printout is not necessary.
There was a problem hiding this comment.
We need this printout for the EL variant. The in-memory thing doesn't even have/need an explicit bootstrap - all realms are implicitly created, as many as you want (it's not intuitive and I think that behavioral difference should really be changed).
There was a problem hiding this comment.
secrets are not stored in EclipseLink anyway
The problem is, that it is possible - and before this change the clear text secrets were passed down to EL and therefore the database, which is IMHO an absolute no-go. But even if it was not the case, the fact that it could happen is not good.
There was a problem hiding this comment.
Yes, I agree that it's a good idea to remove secret fields from JPA classes
There was a problem hiding this comment.
BTW: I've intentionally chosen System.out over LOGGER - it's not yet great though.
There was a problem hiding this comment.
We need this printout for the EL variant.
EL bootstrapping is already impossible without env. vars. Isn't this what #461 attempts to fix?
I believe the discussion there was moving away from printouts. I think it would be safer to remove secret JPA fields first (this does not make the bootstrapping situation worse) and then deal with the bootstrap command in a separate PR. (My preference would be writing generated secrets to a file.)
There was a problem hiding this comment.
Yes. I thought, you could get random secrets.
The whole bootstrap scenario is too complex overall. The In-mem thing bootstraps everything and logs - the EL thing does not log, but can generate creds (which you cannot get)... And quite some if special cases that make it complex to reason about the bootstrap process.
Clear text secrets must never be persisted, hence the attributes in `ModelPrincipalSecrets` needs to be removed.
snazy
force-pushed
the
no-way-to-persist-secrets
branch
from
20a9389 to
c05d02f
Compare
Clear text secrets must never be persisted, hence the attributes in
ModelPrincipalSecretsneeds to be removed.