Project

General

Profile

Actions

Task #3920

closed

Convert existing validator implementations to v2::Validator

Added by Alex Afanasyev about 7 years ago. Updated over 6 years ago.

Status:
Closed
Priority:
High
Assignee:
Category:
Security
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:

Description

Goals:

  • Before TrustSchemaPolicy is implemented, this conversion would allow removal of all v1 keychain and validation code
  • The commit should also remove the unused code (e.g. ValidatorRegex)
  • For existing use of v1 validator in ndn-cxx, convert to v2 validator

The v2::Validator would take ValidationPolicy to check whether signed data/interest satisfy the validation policy.

ValidationPolicy:

  • ValidationPolicyAcceptAll: accept all signed interest/data without checking
  • ValidationPolicyCommandInterest: to check the timestamp of a stop-and-wait command Interest.
  • ValidationPolicySimpleHierarchy: check the signature using a simple hierarchy trust model. The validator would directly fetch certificates according to KeyLocator.
  • ValidationPolicyConf: enable the naming convention check based on configure

In some cases, we may want more complex and fine-grained trust model when validating data packets and signed interest packets. 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.

The outer policy can do policy check based on its own rules and delegate the rest part of the check to inner policy. If a packet cannot pass the outer ValidationPolicy, it won't be passed to inner ValidationPolicy.

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.


Related issues 4 (0 open4 closed)

Related to ndn-cxx - Task #3289: Validator RefactoringClosedAlex Afanasyev10/26/2015

Actions
Blocks NFD - Task #4089: Adapt to ndn-cxx v2::KeyChain and ValidatorClosedJunxiao Shi05/24/2017

Actions
Blocks ndn-tools - Feature #4205: Restore PIB service after ndn-cxx security v2 and other breaking changesRejected

Actions
Blocks ndn-cxx - Task #4246: Remove v1 security frameworkClosedAlex Afanasyev

Actions
Actions #1

Updated by Alex Afanasyev about 7 years ago

  • Subject changed from Convert existing validator to v2::ValidationPolicy to Convert existing validator implementations to v2::ValidationPolicy
Actions #2

Updated by Zhiyi Zhang almost 7 years ago

  • Subject changed from Convert existing validator implementations to v2::ValidationPolicy to Convert existing validator implementations to v2::Validator
  • Description updated (diff)
Actions #3

Updated by Zhiyi Zhang almost 7 years ago

  • Description updated (diff)
Actions #4

Updated by Junxiao Shi almost 7 years ago

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.

Actions #5

Updated by Junxiao Shi almost 7 years ago

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.

Actions #6

Updated by Junxiao Shi almost 7 years ago

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.

Actions #7

Updated by Zhiyi Zhang almost 7 years ago

  • Related to Task #3289: Validator Refactoring added
Actions #8

Updated by Zhiyi Zhang almost 7 years ago

  • Status changed from New to In Progress
Actions #9

Updated by Zhiyi Zhang almost 7 years ago

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.

Actions #10

Updated by Zhiyi Zhang almost 7 years ago

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

Actions #11

Updated by Zhiyi Zhang almost 7 years ago

  • Description updated (diff)
Actions #12

Updated by Junxiao Shi almost 7 years ago

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.

Actions #13

Updated by Junxiao Shi almost 7 years ago

  • Blocks Task #4089: Adapt to ndn-cxx v2::KeyChain and Validator added
Actions #14

Updated by Zhiyi Zhang almost 7 years ago

// 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.

Actions #15

Updated by Zhiyi Zhang almost 7 years ago

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.

Actions #16

Updated by Junxiao Shi almost 7 years ago

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.

Actions #17

Updated by Zhiyi Zhang almost 7 years ago

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.

Actions #18

Updated by Junxiao Shi almost 7 years ago

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);
Actions #19

Updated by Zhiyi Zhang almost 7 years ago

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>)
Actions #20

Updated by Junxiao Shi almost 7 years ago

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>.

Actions #21

Updated by Davide Pesavento almost 7 years ago

  • Priority changed from Normal to Immediate

Raising priority, as this is blocking #4089 which has immediate priority.

Actions #22

Updated by Zhiyi Zhang almost 7 years ago

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.
Actions #23

Updated by Junxiao Shi almost 7 years ago

  • Priority changed from Immediate to High
Actions #24

Updated by Zhiyi Zhang almost 7 years ago

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/

Actions #25

Updated by Junxiao Shi almost 7 years ago

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

Actions #26

Updated by Junxiao Shi almost 7 years ago

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.

Actions #27

Updated by Zhiyi Zhang almost 7 years ago

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.

Actions #28

Updated by Junxiao Shi almost 7 years ago

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.

Actions #29

Updated by Junxiao Shi almost 7 years ago

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

Actions #30

Updated by Zhiyi Zhang almost 7 years ago

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.

Actions #31

Updated by Zhiyi Zhang almost 7 years ago

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
Actions #32

Updated by Junxiao Shi almost 7 years ago

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.

Actions #33

Updated by Eric Newberry over 6 years ago

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.

Actions #34

Updated by Junxiao Shi over 6 years ago

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.

Actions #35

Updated by Eric Newberry over 6 years ago

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.

Actions #36

Updated by Junxiao Shi over 6 years ago

  • Blocks Feature #4205: Restore PIB service after ndn-cxx security v2 and other breaking changes added
Actions #37

Updated by Alex Afanasyev over 6 years ago

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?

Actions #38

Updated by Alex Afanasyev over 6 years ago

  • Blocks Task #4246: Remove v1 security framework added
Actions #39

Updated by Alex Afanasyev over 6 years ago

  • Status changed from In Progress to Closed
  • % Done changed from 0 to 100
Actions

Also available in: Atom PDF