Set bumpalo version for fuzzing#532
Conversation
Cargo.toml
Outdated
| ed25519-compact = { version = "1", default-features = false, optional = true } | ||
| # We import to explicitly lock the version. | ||
| # Remove these dependencies once CTAP is a library. | ||
| serde_json = { version = "=1.0.69", default-features = false, features = ["alloc"] } |
There was a problem hiding this comment.
Why do we need this? We should not depend on serde unless possibly for "std". But then in that case we should make this dependency optional and add it only for "std".
There was a problem hiding this comment.
It's another case were I had to lock the version because of compilation issues. I can probably move it to dev-dependencies, let me try.
There was a problem hiding this comment.
But I don't understand where do we use serde_json?
There was a problem hiding this comment.
#407 is where I added the serde version. After running cargo update as described there, desktop tests still pass.
There was a problem hiding this comment.
Yes, I've seen that PR and would have mentioned that serde_json doesn't need to be added there because we shouldn't depend on it.
I just did the following without issues:
./reset.sh
# Remove serde_json from Cargo.toml
./setup.sh
./run_desktop_tests.sh
So I'm really curious why we need serde_json in the ctap2 crate.
There was a problem hiding this comment.
In both stable and develop, serde is part of the initial commit for libraries/crypto. Copied from stable:
serde = { version = "1.0", optional = true, features = ["derive"] }
serde_json = { version = "1.0", optional = true }
I never checked if we actually need these dependencies, but I removed all of them and it still seems to work. Interesting.
There was a problem hiding this comment.
The crypto library uses them for tests (wycheproof). But ctap2 doesn't.
There was a problem hiding this comment.
Oh, we responded at the same time it seems. I had the same conclusion, so let's try to remove serde!
| regex = { version = "1", optional = true } | ||
|
|
||
| [features] | ||
| std = ["hex", "ring", "rng256/std", "untrusted", "serde", "serde_json", "regex"] |
There was a problem hiding this comment.
I think this file was correct. We use serde_json for tests.
There was a problem hiding this comment.
Ah, I see. In that case, I think I had random issues with the serde version before I added it to the main Cargo.toml as well. Not sure how to reproduce, so maybe we can just remove it outside of crypto and try again until that happens.
There was a problem hiding this comment.
Ah, I see. In that case, I think I had random issues with the serde version before I added it to the main Cargo.toml as well. Not sure how to reproduce, so maybe we can just remove it outside of crypto and try again until that happens.
There was a problem hiding this comment.
Yes, now the change is making sense.
I think it was hard to reproduce because of some many states (the Cargo.lock files in multiple directories). The best way to test is ./reset.sh (committing code first otherwise it's lost) but it can be a bit slow for fast iteration.
* Add basic setup for multi-PIN
- Reserve the storage keys for maximum of 8 user slots.
- Modify the storage functions to take a slot_id parameter.
- Add the slot_count() customization.
- Assume slot_id as a parameter when needed except these places:
- Entrance functions of command processing that directly takes the
command parameter structure. slot_id is set as 0, and will be
parsed from the parameters when we enable the feature.
- MakeCredential/GetAssertion/AuthenticatorConfig will take the
slot_id from active token state when we enable the feature,
resulting in an `Option<usize>`. Below code will act on the option
value correctly. When the feature isn't enabled, we're always
referring to the only PIN slot so set slot_id as Some(0).
- GetInfo returns verdict of whether PIN is supported and enabled, and
whether PIN needs to be forced changed. There will be new fields to
represent those values when the feature is enabled, and the old
fields will not be populated. So when the feature isn't enabled, we
can treat slot_id as 0.
Not covered in this commit:
- Unittests for other slots. The existing tests all pass and I plan to
add unittests for multi-slot case after the codebase allows enabling
the feature.
- Persisting and checking the slot_id in credentials. This is planned to
come in the next commit.
* Fix storage and some other style
* Add support for concatenated values
* Switch some storage entries back to multi-entry
* Set bumpalo version for fuzzing (#532)
* maximum working bumpalo version
* explicit comment to explain version locking
* removes incorrect comment
* moves serde version lock to dev dependencies
* removes serde dependencies
* reverts serde removal in crypto library
* Make PIN_PROPERTIES use concatenated storage entry
* Fix bumpalo issue
* Use concatenated storage entry for force_pin_change too
* Fix cargofmt
Co-authored-by: Julien Cretin <cretin@google.com>
Co-authored-by: kaczmarczyck <43844792+kaczmarczyck@users.noreply.github.com>
2 participants
Fixes CI problems.