NFD » ndn-cxx

What's the relation between this issue and #1872, #3289?
If they are duplicates, mark as so.
If they are not longer needed, abandon these issues.
Otherwise, explain.

To enable more complex and flexible policy, we introduce inner policy as the member variable of ValidationPolicy. This inner policy enables the combination of multiple policies.

What's the order of policy evaluation? Is the outer policy checked first, or the inner policy checked first?
The example below indicates that the outer policy is checked first, and the inner policy is considered only if the packet passes outer policy. However, this point should be clarified in the design, and not just given in an example.

e.g. One can use ValidationPolicyCommandInterest to check the signed interest's timestamp. To check the remainder parts (signatures of data packets and interest packets), one can use ValidationPolicySimpleHierarchy as the inner policy of ValidationPolicyCommandInterest. In this way, all data packets and interests that satisfy the ValidationPolicyCommandInterest would then use ValidationPolicySimpleHierarchy to finish the remainder checking.

This example design is flawed. It has the same problem as #3821.
Instead, signature must be verified before timestamp; or, "latest timestamp" updating should be deferred after the packet passes signature checking.

In https://gerrit.named-data.net/3787 patchset5 I see two possible patterns on constructing a validator with a chain of policies:

// A
auto policy1 = make_unique<Policy1>();
auto policy2 = make_unique<Policy2>();
policy1->setInnerPolicy(std::move(policy2));
auto validator = make_unique<Validator>(std::move(policy1), certFetcher);

// B
auto policy1 = make_unique<Policy1>();
auto policy2 = make_unique<Policy2>();
auto validator = make_unique<Validator>(std::move(policy1), certFetcher);
validator->getPolicy().setInnerPolicy(std::move(policy2));

Both seem correct, but they result in different internal states:

• Pattern A: validator->getPolicy().getInnerPolicy().m_validator == validator.
• Pattern B: validator->getPolicy().getInnerPolicy().m_validator == nullptr.

Also, both patterns are verbose.
It is also unclear what is the correct behavior when the policies are changed during a validation.

I suggest adopting a single pattern for this construction:

// C
auto validator = make_unique<Validator>(
  make_unique<Policy1>()->chain(make_unique<Policy2>()),
  certFetcher);

class ValidationPolicy
{
public:
  /** \brief set inner policy
   *  \pre setValidator has not been invoked
   *  \pre no inner policy is set
   *  \return inner policy
   */
  ValidationPolicy&
  chain(unique_ptr<ValidationPolicy> inner);
};

Pattern C can allow arbitrary chaining, and ensure all chaining are constructed before the Validator.
Chaining is achieved using chain method instead of passing inner policy to the constructor, which eliminates the requirement on every subclass to add such constructor.

Junxiao Shi wrote:

To enable more complex and flexible policy, we introduce inner policy as the member variable of ValidationPolicy. This inner policy enables the combination of multiple policies.

What's the order of policy evaluation? Is the outer policy checked first, or the inner policy checked first?
The example below indicates that the outer policy is checked first, and the inner policy is considered only if the packet passes outer policy. However, this point should be clarified in the design, and not just given in an example.

The outer policy will check first and delegate rest part of validation to inner policy. I would update the design.

e.g. One can use ValidationPolicyCommandInterest to check the signed interest's timestamp. To check the remainder parts (signatures of data packets and interest packets), one can use ValidationPolicySimpleHierarchy as the inner policy of ValidationPolicyCommandInterest. In this way, all data packets and interests that satisfy the ValidationPolicyCommandInterest would then use ValidationPolicySimpleHierarchy to finish the remainder checking.

This example design is flawed. It has the same problem as #3821.
Instead, signature must be verified before timestamp; or, "latest timestamp" updating should be deferred after the packet passes signature checking.

You are right. I would change the example and update ndn-cxx Change 3645.

I agree. Use a chain function is fine. I will make the change.

e.g. To check the stop-and-wait command interest, the packet needs to satisfy 1. signature validation rules and 2. timestamp check. In this case, one can create a ValidationPolicySimpleHierarchy as the outer check and set the inner ValidationPolicy to be ValidationPolicyCommandInterest. With the combined ValidationPolicy, the signature of incoming packets would first be checked by outer policy and the timestamp of command interest would then be checked by inner policy.

The statement "the signature of incoming packets would first be checked by outer policy" is wrong.
ValidationPolicySimpleHierarchy only checks the KeyLocator of the signing key conforms to the policy, but does not perform cryptographic verification of the signature bits.
Thus, merely putting ValidationPolicyCommandInterest inside ValidationPolicySimpleHierarchy does not solve #3821.

What we need is a mechanism for the Validator to inform the policy the outcome of cryptographic verification.
ValidationPolicyCommandInterest should update its LastTimestampRecord only after cryptographic verification is successful.
Also, ValidationPolicyCommandInterest should not accept a new validation request until the cryptographic verification of the previous validation request is completed. Otherwise, it would not be able to enforce the timestamp is monotonically increasing.

// C
auto validator = make_unique<Validator>(
  make_unique<Policy1>()->chain(make_unique<Policy2>()),
  certFetcher);

class ValidationPolicy
{
public:
  /** \brief set inner policy
   *  \pre setValidator has not been invoked
   *  \pre no inner policy is set
   *  \return inner policy
   */
  ValidationPolicy&
  chain(unique_ptr<ValidationPolicy> inner);
};

For the design here. I feel the chain function is in the wrong direction.

make_unique<Outer Policy>()->chain(make_unique<Inner Policy>() 

This would return the inner policy. However, the validator should use the outer policy as the parameter, for the ValidationPolicy::setValidator() function would recursively set validator from outer to inner. If we change the direction like:

make_unique<Inner Policy>()->chain(make_unique<Outer Policy>() -> chain(make_unique<Outer outer policy>)

Then it would finally return the outermost policy to the validator.

Junxiao Shi wrote:

What we need is a mechanism for the Validator to inform the policy the outcome of cryptographic verification.
ValidationPolicyCommandInterest should update its LastTimestampRecord only after cryptographic verification is successful.
Also, ValidationPolicyCommandInterest should not accept a new validation request until the cryptographic verification of the previous validation request is completed. Otherwise, it would not be able to enforce the timestamp is monotonically increasing.

This is not a small change. We need more discussion.

For the design here. I feel the chain function is in the wrong direction.

make_unique<Outer Policy>()->chain(make_unique<Inner Policy>() 

This would return the inner policy.

Yes, I acknowledge this error.

However, the validator should use the outer policy as the parameter, for the ValidationPolicy::setValidator() function would recursively set validator from outer to inner. If we change the direction like:

make_unique<Inner Policy>()->chain(make_unique<Outer Policy>() -> chain(make_unique<Outer outer policy>)

Then it would finally return the outermost policy to the validator.

It would be counter-intuitive to write the inner policy first. But renaming chain to wrapIn can make it clear that inner policy goes first.

Alternatively, learn from how ndn::security::transform achieves chaining. It's slightly better but more complicated.

Junxiao Shi wrote:

It would be counter-intuitive to write the inner policy first. But renaming chain to wrapIn can make it clear that inner policy goes first.

The problem here is regarding the implementation. Given the parameter is unique_ptr<VlidationPolicy> outerPolicy, I have no clue how to make outerPolicy->setInnerPolicy(this?) happen. One way is to move the function out of the class, but in that way, it won't looks like make_unique<InnermostPolicy>()->...->wrapIn(make_unique<OutermostPolicy>() any more.

The problem here is regarding the implementation. Given the parameter is unique_ptr<VlidationPolicy> outerPolicy, I have no clue how to make outerPolicy->setInnerPolicy(this?) happen.

This issue is applicable regardless of naming: chain or wrapIn has no way to return unique_ptr<ValidationPolicy>.
One solution is to let the Validator itself do the chaining:

/** \brief construct a validator with a chain of policies
 *  \param policies a chain of validation policies, from outer to inner;
 *                  this chain MUST NOT be empty
 */
Validator(std::initializer_list<unique_ptr<ValidationPolicy>> policies,
          unique_ptr<CertificateFetcher> certFetcher);
// This constructor calls a private static function to chain the policies, and then delegates to the other constructor.
// Empty chain should cause an assertion failure.

// invocation
make_unique<Validator>({
    make_unique<OuterPolicy>(),
    make_unique<MiddlePolicy>(),
    make_unique<InnerPolicy>(),
  }, certFetcher);

How about this way:

class ValidationPolicy
{
  ValidationPolicy&
  chain(unique_ptr<ValidationPolicy> innerPolicy)
  {
    if (m_innerPolicy == nullptr) {
      m_innerPolicy = std::move(innerPolicy);
    }
    else
      m_innerPolicy->chain(std::move(innerPolicy));
    return *this;
  }
}

So that we can still use the pattern like:

make_unique<Outermost Policy>()->chain(make_unique<Middle Policy>() -> chain(make_unique<Inner policy>)

note-19 design has exactly the same problem: The return value of chain is not unique_ptr<ValidationPolicy>, but Validator constructor wants unique_ptr<ValidationPolicy>.

Zhiyi Zhang wrote:

Junxiao Shi wrote:

note-19 design has exactly the same problem: The return value of chain is not unique_ptr<ValidationPolicy>, but Validator constructor wants unique_ptr<ValidationPolicy>.

It's hard to chain unique_ptr together in the initialier_list, because unique_ptr has implicitly-deleted copy constructor. Anyone has a solution?

Also make_unique function cannot support initializer_list or vector as the parameter.

no matching function for call to 'make_unique'

ndn-cxx/src/util/backports.hpp:38:1: note: candidate function not viable: requires single argument 'args', but 2 arguments were provided
make_unique(Args&&... args)
^
1 error generated.

It's not viable to use an initializer_list to hold unique_ptr. Because initializer_list only allow const access to its member.

https://stackoverflow.com/questions/8193102/initializer-list-and-move-semantics

Therefore, I got a compromised way: https://gerrit.named-data.net/#/c/3787/

How about other container types such as std::vector?

Looking back: I wonder what's the use case for "inner policy", other than CommandInterest validation?
If there aren't any other use case, there's no need for a "general chaining interface". We can just let ValidationPolicyCommandInterest constructor take an inner policy, and don't bother about this elsewhere.

Junxiao Shi wrote:

Looking back: I wonder what's the use case for "inner policy", other than CommandInterest validation?
If there aren't any other use case, there's no need for a "general chaining interface". We can just let ValidationPolicyCommandInterest constructor take an inner policy, and don't bother about this elsewhere.

For now, seems we only have this use case for command interest validation. The question is we don't know if we need it in the future. e.g. a new kind of signature or command interest format.

The question is we don't know if we need it in the future. e.g. a new kind of signature or command interest format.

Nobody can predict the future. Given we can't find a good design for chaining and there's no known use case, we shouldn't waste more time on a generic chaining interface.

https://gerrit.named-data.net/3787 patchset8 adds ValidationPolicy::m_validator member.
I wonder what's the use case for this?

Junxiao Shi wrote:

https://gerrit.named-data.net/3787 patchset8 adds ValidationPolicy::m_validator member.
I wonder what's the use case for this?

Let Alex answer the question. For now, seems it has not been used in any use case if I am correct.

Junxiao Shi wrote:

This example design is flawed. It has the same problem as #3821.
Instead, the signature must be verified before timestamp; or, "latest timestamp" updating should be deferred after the packet passes signature checking.

I have no clue how to handle it in the current structure. The success callback is immediately called when the cryptographical signature is verified and there is no way to modify the success callback with current interfaces.

We always first check policy then signature. If we want to handle this case, we need to modify the basic logic in the validator. We need more discussion on how to make the change. Some of my ideas:

• instead of using single checkPolicy in validator, we use checkBeforeSigVerificationPolicy and checkAfterSigVerificationPolicy
• change the interface to enable the modification on successCallback in ValidationState

Reply to note-31:

A after-crypto policy is not necessary. For the purpose of stop-and-wait CommandInterest, an after-crypto callback is sufficient. It operates as follows:

1. The command Interest policy checks the timestamp is greater than "last timestamp", but does not update "last timestamp" record.
2. An inner policy checks the certificate chain.
3. The validator verifies signature bits.
4. If the signature is valid, the command Interest policy is invoked in a callback to update "last timestamp" record. In this step, it does not need to check the timestamp again, and has no chance of failing the validation.

As required by "stop-and-wait" protocol, the signer should not send the next command Interest before the last one is completed. When this requirement is violated, the command Interest policy could accept backdated timestamps, but that is not the fault of the command Interest policy.

It looks like the most recent merged change (gerrit 3646) broke NFD, at the very least the code for nfdc. The nfdc code fails to compile because it can't find Validator or ValidatorNull. I can't seem to find a change fixing this, so I wasn't sure if anyone was aware of this yet. I'll look into fixing this, since it might just be a simple namespace change. If it's that simple, I'll go ahead and push a patch.

it might just be a simple namespace change.

There's more than that. See this post for how I'll review NFD Changes in the meanwhile.

Eric Newberry wrote:

It looks like the most recent merged change (gerrit 3646) broke NFD, at the very least the code for nfdc. The nfdc code fails to compile because it can't find Validator or ValidatorNull. I can't seem to find a change fixing this, so I wasn't sure if anyone was aware of this yet. I'll look into fixing this, since it might just be a simple namespace change. If it's that simple, I'll go ahead and push a patch.

I tried to fix this without knowing anything about the new security system. I'll leave the fix to someone more knowledgeable about the changes than I.

https://gerrit.named-data.net/#/c/3648/ is the complete (per my understanding) version of the commit.

There is one behavior change that will impact dependent projects. Old implementation allowed "reset" ValidatorConfig to reload it from scratch. The new one does not have this ability. Do you think it should be re-enabled or we can re-create the validator whenever it needs to be reloaded?