Move three dependent customizations into new file

[hcyang-google]

• default_min_pin_length(_rp_ids) and max_rp_ids_length

• Did some backing store tricks to make the list configurable in
  TestCustomization.

I need some suggestions on implementing the TestCustomization's list field.

[hcyang-google]

@ia0 @kaczmarczyck is there better way of doing this? The current implementation is quite complicated and I feel it's unnecessary.

[ia0]

Yes, I think we should change the Customization API:

// Current PR:
fn default_min_pin_length_rp_ids(&self) -> &[&str];
// Proposal:
fn default_min_pin_length_rp_ids(&self) -> Vec<String>;

The cost of creating the vector seems to already be present in ctap::storage::min_pin_length_rp_ids(). It's not as clear for ctap::storage::set_min_pin_length_rp_ids because if the RP id was already present in the function argument then we recreate it. But it's probably fine since the cost is comparable to the final result.

There are 2 alternatives:

// This isn't yet supported (but will in the future):
fn default_min_pin_length_rp_ids(&self) -> impl Iterator<Item = &str>;
// This would add rp_ids if they are not already there. It could either be called with an empty Vec to get the list, or with the argument to merge. This is essentially the union of both usages.
fn merge_default_min_pin_length_rp_ids(&self, rp_ids: &mut Vec<String>);

[coveralls]

Coverage decreased (-1.4%) to 88.27% when pulling bbc51af on hcyang-google:customization into 74b472d on google:develop.

[hcyang-google]

Add a test to verify that the list configuration implementation isn't broken.

[kaczmarczyck]

This should be 2 tests: One part is testing the current customization, and the other is testing your implementation.

[hcyang-google]

fixed

[hcyang-google]

@ia0 Now we do this, so this customization (and ENTERPRISE_RP_ID_LIST) won't be treated as constants anymore. But I think it should be fine? There's very limited code path affected.

[ia0]

Indeed they look less like constants. But I would be a bit surprised if some optimizations would not trigger. The optimization we rely on is when we iterate on this list in set_min_pin_length_rp_ids. The whole loop should be eliminated. We could try to check that it's still the case. One possible trick is to call an extern function in the loop and see if the linker complains:

    for &rp_id in DEFAULT_MIN_PIN_LENGTH_RP_IDS.iter() {
        // Add the 2 following lines to test if it still links
        extern "C" { fn should_not_be_called(); }
        unsafe { should_not_be_called() };
        let rp_id = String::from(rp_id);
        if !min_pin_length_rp_ids.contains(&rp_id) {
            min_pin_length_rp_ids.push(rp_id);
        }
    }

[hcyang-google]

The optimization won't trigger. I think it's because to eliminate the loop code the function has be able to be computed at compilation time. But we have some heap allocation like String::from in that function which make it impossible to do so?

[ia0]

I don't think the String::from should matter if the list is empty. But yeah, it's possible the constant propagation is not aggressive enough. In particular, if collect is not able to see that the iterator is empty, then we won't get the optimization.

[ia0]

nit: MAX_RP_IDS_LENGTH is now max_rp_ids_length but we could as well do a pass on documentation after the code change if it's easier (there's a few other cases like this one below, I'm not commenting on them to minimize comments to those regarding design).

[hcyang-google]

done. @ia0 are there more that needs to be improved in this CL or can this be approved and merged?

[ia0]

I wanted to test a bit the optimizations to see if there was a way to make it work with Vec<String> too. But actually ever Vec<&'static str> doesn't seem to work. Probably the iter() is too much.

[ia0]

Indeed they look less like constants. But I would be a bit surprised if some optimizations would not trigger. The optimization we rely on is when we iterate on this list in set_min_pin_length_rp_ids. The whole loop should be eliminated. We could try to check that it's still the case. One possible trick is to call an extern function in the loop and see if the linker complains:

    for &rp_id in DEFAULT_MIN_PIN_LENGTH_RP_IDS.iter() {
        // Add the 2 following lines to test if it still links
        extern "C" { fn should_not_be_called(); }
        unsafe { should_not_be_called() };
        let rp_id = String::from(rp_id);
        if !min_pin_length_rp_ids.contains(&rp_id) {
            min_pin_length_rp_ids.push(rp_id);
        }
    }

[ia0]

This is approved on my side, but since it's a big PR, I think we should also wait for @kaczmarczyck to take a look too.

[kaczmarczyck]

I had a comment 5 days ago, and left everything else to you :)
I think the most interesting decision was the change from static strings, and since you are aware, feel free to merge when you think the impact on performance and binary size is small enough.