Don't enable RUST_BOOTSTRAP in build.rs

[roblabla]

The crate is currently "usable" on stable rust, despite using nightly-only feature, by enabling RUST_BOOTSTRAP=1 in build.rs. This is a very bad idea. This flag is only meant to bootstrap the compiler itself, and any other use of it is unsupported and might lead to the code breaking if/when the API changes. This is not unheard of: Nightly features aren't finalized and often changes shape prior to stabilization.

The currently used features are also not very stable, const_generics and const_fn can easily cause ICEs in the compiler or soundness issues in the final binary, due to implementation bugs.

IMO, this crate shouldn't abuse RUST_BOOTSTRAP like this. The correct way is to tell the users to use a nightly compiler. If that's not possible, the use of RUST_BOOTSTRAP should at least be called out in the README so users can make an informed decision about using this crate.

[CasualX]

Hi roblabla, I'm aware of this and I consider it a mistake that I did this in the first place. I was so excited to make this library but I needed one little thing, to be generic over arrays of all sizes. That one puzzle piece has always been missing and at the time I considered cheating with RUSTC_BOOTSTRAP a small price to pay.

From there on forward I considered ways to remove this but I was held back because this would be a 'breaking' change and so I justified to myself continued usage of this backdoor.

I'm actually surprised crates.io accepted my crate and cargo doesn't try to contain this particular env var from being propagated from a build script.

For now this won't go away (unless forced to by eg. cargo no longer propagating this env var via build script). Perhaps open an issue on cargo's issue tracker to see what other people think.

Thanks and congratulations for being the first person to notice this :)

[CasualX]

See rust-lang/cargo#7088 for more information.

[roblabla]

So, this answer was a bit disappointing to hear, as it makes it pretty hard to rely on obfstr on production code. I have forked obfstr 1.1 and, with the use of generic_array managed to make something working reasonably well on stable without the need for any nightly feature. See roblabla@195db0c

Only real downside is that it makes obfconst slightly less ergonomic to use:

static GSTR: obfstr::ObfString<[u8; 10]> = obfstr::obfconst!("Hello 🌍");
use generic_array::{GenericArray, typenum::consts::*};
static GSTR: obfstr::ObfString<GenericArray<u8, U10>> = obfstr::obfconst!("Hello 🌍");
assert_eq!(GSTR.decrypt(0).as_str(), "Hello 🌍");

And, as with anything relying on generic-array, the errors that occur when giving the wrong output size are pretty bad if you don't know how to read them:

---- src/lib.rs - obfconst (line 72) stdout ----
error[E0308]: mismatched types
 --> src/lib.rs:74:56
  |
5 | static GSTR: obfstr::ObfString<GenericArray<u8, U9>> = obfstr::obfconst!("Hello 🌍");
  |                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `obfstr::typenum::B0`, found struct `obfstr::typenum::B1`
  |
  = note: expected struct `obfstr::ObfString<obfstr::GenericArray<_, obfstr::typenum::UInt<obfstr::typenum::UInt<_, obfstr::typenum::B0>, obfstr::typenum::B1>>>`
             found struct `obfstr::ObfString<obfstr::GenericArray<_, obfstr::typenum::UInt<obfstr::typenum::UInt<_, obfstr::typenum::B1>, obfstr::typenum::B0>>>`
  = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

So my question is this: are you open to moving to such a solution? If not, I guess I'll publish my fork on crates.io with another name (like obfuscate or something).

[CasualX]

I have a very strong dislike for GenericArray as it makes something that should be simple so complicated (I know it isn't anyone's fault and Rust wants to make sure that adopting new features are solid).

I'm also moving past the proc-macro honeymoon phase and seeing the extra crate requirement as a serious downside, hence in v0.2 I implemented the whole API in const fn (but again needing nightly, unstable and unfinished features).

THAT SAID:

Since I've moved on from v0.1 and don't care about it anymore I have no problems publishing a v0.1.2 with is a carbon copy of your PR (but not merged into master). Basically a blessed branch which is kept updated and I don't have a problem making this choice explicit in the readme.

Technically this would be a breaking change (you pointed out obfconst) but I hope not too many people have a need for using this API directly as it was hard to use regardless.

I'll reopen this issue and we can discuss the details of how to best merge your work. Thanks for taking interest and caring, it really is appreciated.

[roblabla]

I have a very strong dislike for GenericArray as it makes something that should be simple so complicated (I know it isn't anyone's fault and Rust wants to make sure that adopting new features are solid).

Haha, I'm also not a fan of GenericArray, but in this case I couldn't find any other way to make it work, given the constraints ^^.

Technically this would be a breaking change (you pointed out obfconst) but I hope not too many people have a need for using this API directly as it was hard to use regardless.

https://github.com/search?l=Rust&q=obfconst&type=Code shows a single user in open source repo. That said, I expect most users of obfstr to have closed source code, so it's kinda hard to gauge. In my own code, I used that function once in order to share an obfuscated string between multiple sites.

Something I could do to mitigate breakage is allow ObfString<[u8; N]> for N < some sane value (like 128). This would make existing code using small enough strings work. The downside is, I'll probably have a macro generate a ton of code to support this, which would slow compilation down.

Another thing I also thought about: It might be worth adding a new macro (tentatively named obfstatic), meant to replace obfconst's main use-case working like this:

// Generates static GSTR: ObfString<GenericArray<u8, "test".len()>> = ObfString::new(_random_!(), "test");
obfstatic!(GSTR = "test");
fn test() {
    println!("{}", GSTR.decrypt());
}

The idea is to hide the underlying type within the obfstr from the user as much as possible. It has the additional nice trait that it lets the user omit the string length 😄 . If the same macro is implemented for 0.2, it would allow for easy migrations between both versions without changing any code.

Thanks for taking interest and caring, it really is appreciated.

Thanks for the great crate - it's really nice!

[CasualX]

Perhaps the syntax can more closely mimic static/const syntax (and suppport visibility rules in the process):

obfconst! {
    pub const GSTR = "test";
}

I've made a branch with such an experiment for v0.2: obfconst

Something I could do to mitigate breakage is allow ObfString<[u8; N]> for N < some sane value (like 128).

I think this is a good idea.

shows a single user in open source repo

Reverse dependencies on crates.io shows another user (usage in its build script, ctrl-f obfstr)

Looking at these example shows questionable usage of obfstr in order to hide credentials in the final binary which is not something I had in mind when I made this crate... Oh well I guess I should have known better >.<

Thanks for the great crate - it's really nice!

Thanks!

[roblabla]

So, I finally got some more time to work on this. Unfortunately, I sort of hit a roadblock on my current idea using GenericArray with a compat layer for "old-style" arrays, that I don't think I'll be able to solve. The crux of the problem is, I would need the arr! macro to generate either a GenericArray of a raw array depending on the context it's in. And it can't even know what it's supposed to generate, since it's all based on type inference. So, back to the drawing board.

I've got a solid new idea for a layer that would drop the GenericArray requirement, work on stable, and basically be compatible with most of the API surface of the 0.1.x API series. The only breaking change needed, I think, is making ObfString::new() unsafe. The rest of the API would stay entirely untouched, only the internal implementation would change.

Here's the idea: Change ObfString::new to be an unsafe function of signature pub const unsafe fn new(key: u32, data: A) -> ObfString<A>;. The user must pass a byte array in data.

The decrypt function will then cast data to a slice of the correct size with:

let data = {
    // Safety: ObfString::new guarantees that the input type is a byte array.
    slice::from_raw_parts(&self.data as *const A as *const u8, mem::size_of::<A>())
}

ObfBuffer::as_str() will similarly cast to a slice with the same function.

---

In practice, I don't think anyone uses ObfString::new() function directly. So while it's technically a breaking change, I expect actual breakage to be nil.

A PR will go up soon.

[CasualX]

Thanks for your work! I'll be available in the weekend to work through your PR.

I'll also set up a separate 0.1.x branch because merging to master won't be possible.

I don't think anyone uses ObfString::new() function directly.

If they are, they're not supposed to as it is intended to be only used by the macro. This approach may work.

Rust also recently stabilized some trait implementations for all array sizes (feature min_const_generics). This means that it may be possible to use the constraint A: AsRef<[u8]> to implement the conversion to slice and avoid unsafe code.

[CasualX]

Hey @roblabla if it's okay I implemented my proposal using AsRef and AsMut instead of GenericArray. I also created a separate stable0.1 branch to track updates. Future PRs I would like to add better notifications to the 0.2 series referring to this branch and actually publishing newer 0.1 versions.

[roblabla]

I believe the only thing missing to close this issue is a 0.1.2 release on crates.io 🎉

[CasualX]

It's not that simple, I need to make new PR updating the version number for the proc-macro and the this crate. It's so easy to screw up. I think the proc-macro needs to be a 0.2.0 because from its perspective it is a breaking change, but I only need to update obfstr itself to 0.1.2. I need to not forget to update the proc-macro dependency.

I really dislike updating more than one crate in sync. For all its glory in making it hard to make programming mistakes Cargo has shortcomings when it comes to preventing accidental versioning mistakes... I'll get to it when I find the courage to think about this in detail.

[roblabla]

Rust 1.51.0 is released now. I believe this means obfstr 0.2.0 can now work on stable without any hacks, all that's required is removing the build.rs and the feature flag. Doing so will bump the MSRV though, so it's probably wise to wait a bit before doing this change :).

[CasualX]

I'll update the repo soon (tm) and wait a week or so before publishing. Personally I have no intention to support anything other than latest stable.