Make SecretBuffer and shred! public

[PallHaraldsson]

I'm assuming can and should (sometimes) be used, not just by Julia.

[Was it ok where put the public declaration under: shred! ok under Strings? Maybe it or both should go under IO? Where how no effect, except sort of [mis]documenting, the main thing is if this should be made public at all.]

[jakobnissen]

Wouldn't this be better in a package? To me this seems like a prime example of functionality that maybe shouldn't have been in Base

[vtjnash]

Good point. We needed this in stdlib/LibGit2, so would you want to make this a new stdlib (./contrib/new-stdlib.sh) and move the getpass and SecretBuffer code there instead?

[jakobnissen]

I'll give it a go

[KristofferC]

No way we make a whole new stdlib for this.

[PallHaraldsson]

A fair amount of packages already use it and https://juliahub.com/ui/Search?type=code&q=shred! 447 hits, so seems too late to back out of having it (despite it never an official API, is it known to be bad/ly designed or anything, or only that it should maybe never have lived in Base?) or move. Yes, a stdlib may be overkill unless it lives in stdlib/LibGit2 (similar to download should have not been in Base). Should it be deprecated in Base and/or moved? I don't care much, I just felt it might be usable as is, and public API warranted, even more now I see it used. Can it be moved still have forwarder wrapper in Base? Isn't it then as good to leave it there? Could do public AND deprecate. And postpone moving. I don't know yet how much code this is, any burden to have it in Base and/or sysimage?

Should I add public for Base.getpass and then also for:

on Windows, the secret might be displayed as it is typed; see Base.winprompt

I didn't check yet if either or both used in packages, or if only the other two make sense. [I changed to draft just because I'm unsure about those other, that could be same or separate PR. Not sure how to add "DO NOT MERGE" yet label or if I can.]

[jakobnissen]

Hm yeah that's a lot of usage in the ecosystem. It was never documented in the manual, so removing it is fair game. That being said, I agree the polite thing would be to deprecate it first. Maybe have it be deprecated in 1.13, then remove it in 1.14?
One option is to move it to a normal package and then vector it in the LibGit2 stdlib.

[mbauman]

I don't really want to see this become public, personally. It's true the only reason this exists in the first place was to get LibGit2 working for v1.0 (#27565). It's a little kludgy, and there's no other reason it needs to be in base.

I'd much rather see it split into an external package.

[PallHaraldsson]

Maybe have it be deprecated in 1.13,

(soft) deprecate right away in 1.12? But not remove it, even ever, just document a substitute package when we have it, at least if that happens or has better API or implementation, or even the same.

It's a little kludgy, and there's no other reason it needs to be in base.

I think you're selling yourself short and the need for it in Base. People need to be able to write secure code, more than do math, e.g. have atan or have BLAS, and the rather small code is already there to help with that. Another reason to have anything in Julia is if used by Julia itself, and sort of true here; for LibGit2, you could say it shouldn't need to be bundled with Julia and be right, still the code here, shred! etc, could be bundled...

What's the kludge (and is it in the implementation, at least the "API" itself ok?), since you wrote it... and it seems to need to be in Base, since a lot of packages depend on it? So does is have a snowball change of being split into a package? I also think we could make it public AND deprecate it...

An IOBuffer-like object where the contents will be securely wiped when garbage collected.

Isn't that really needed (for anyone doing such correctly?), and while yes, could be a package, if would need to do the same, more or less all the functions. People can shred! regular strings (if careful, might do it incorrectly since UTF-8 is variable length), but it's very likely most users might not and would screw up the security, so at least the correct way needs to be very discoverable. I note (so why not make "best practice" API?):

It is considered best practice to wipe the buffer using Base.shred!(::SecretBuffer) as
soon as the secure data are no longer required.

    SecretBuffer(str::AbstractString)
A convenience constructor to initialize a `SecretBuffer` from a non-secret string.
Strings are bad at keeping secrets because they are unable to be securely
zeroed or destroyed. Therefore, avoid using this constructor with secret data.

[Because they are immutable? You mean otherwise you could shred! them?]

Interesting, and I find it unlikely people would implement this right if on their own:

# Unlike other IO objects, equality is computed by value for convenience
==(s1::SecretBuffer, s2::SecretBuffer) = (s1.ptr == s2.ptr) && (s1.size == s2.size) && (UInt8(0) == _bufcmp(s1.data, s2.data, min(s1.size, s2.size)))
# Also attempt a constant time buffer comparison algorithm — the length of the secret might be
# inferred by a timing attack, but not its values.
@noinline function _bufcmp(data1::Vector{UInt8}, data2::Vector{UInt8}, sz::Int)
..
# All SecretBuffers hash the same to avoid leaking information or breaking consistency with ==
const _sb_hash = UInt === UInt32 ? 0x111c0925 : 0xb06061e370557428

This is needed but needs not be part of the public API (not "used" by any package, or almost; only for/by precompile statement of IonCLI):

function final_shred!(s::SecretBuffer)

https://github.com/Roger-luo/IonCLI.jl/blob/ec653d16cb7c99cad1b7868daeba90412a26c9c5/deps/statements.jl#L250

I doubt this should be made public (only used by BinaryBuilder tests):
Not sure yet if this is used or should also be made public:

isshredded(s::SecretBuffer) = all(iszero, s.data)

[mbauman]

What's the kludge (and is it in the implementation, at least the "API" itself ok?), since you wrote it... and it seems to need to be in Base, since a lot of packages depend on it?

Here's why: it does not have a well-defined security boundary or threat model. The binding that should be publicized instead, IMO, is Base.securezero!. That's the only special thing SecretBuffer uses.