Skip to content

Conversation

jaczhi
Copy link

@jaczhi jaczhi commented May 4, 2025

Allow dv to interpret Prefix Insertion requests.

Prefix Insertion protocol: https://gist.github.com/jaczhi/5408716346761de953bec18444b9daf4

Tested using https://github.com/jaczhi/prefix-insertion-test/ (will write the ndnd client in a later PR)

@zjkmxy zjkmxy requested a review from Copilot May 4, 2025 18:18
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for Prefix Injection in dv by extending the protocol handling and configuration across multiple modules. Key changes include:

  • Updating client trust validation to support a default fetch function for certificate retrieval.
  • Introducing new constants, types, and API endpoints for handling prefix injection.
  • Integrating prefix injection logic into the DV router and adding corresponding configuration options.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
std/object/client_trust.go Updated trust validation to use a default fetch function.
std/ndn/mgmt_2022/route.go Added a new route origin constant for prefix injection.
std/ndn/mgmt_2022/definitions.go Introduced new types for Prefix Injection message formatting.
std/ndn/constants.go Added a constant for the new content type Prefix Injection.
std/ndn/client.go Extended ValidateExtArgs to include a fetch function for certificate retrieval.
dv/dv/router.go Integrated prefix injection handler registration and configuration.
dv/dv/injection.go Implemented the core logic for processing Prefix Injection requests.
dv/dv.sample.yml Updated sample configuration with prefix injection options.
dv/config/config.go Added new configuration fields for prefix injection support.

overrideName = args.OverrideName
}

// Default fetch function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we fetch certificates by default?
The current Object client does not have a deadline system, which means it may be vulnerable to infinite fetching loop. Need to look into this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My old design in NTSchema is that the lifetime of a certificate fetching Interest is the original Interest's lifetime minus time elapsed till the send time, so that the validation will fail when the lifetime of original Interest for the Data packet is reached.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, unsure what you mean? Currently this API is used by other code and thus I wanted to maintain the backwards compatibility, just added a new functionality of being able to override the fetch function. (Currently that other code expects ValidateExt to fetch recursively I believe)

@Pesa
Copy link
Member

Pesa commented May 4, 2025

I thought we still had a couple of open questions on the protocol. Have those been resolved?

@jaczhi
Copy link
Author

jaczhi commented May 4, 2025

@Pesa I believe we informally stabilized on a few conclusions for the questions brought up:

how does a data producer know if there is a "routing entity" running locally? -> no easy way to do this, recommend implementers or users to also send a rib/register to the connected forwarder

do not use keyword components unecessarily in interest -> removed from the protocol

how to staple certs -> followed Junxiao's suggestion to move stapling outside of the Data TLV

reuse the existing Prefix Announcement object -> prefix announcement does not have cost as its semantics are different, thus use a similar object but not the same.

how do /routing/inject interests reach the routing entity? what if there are 2+ routing entities connected to the same producer? -> currently this question does not block the implementation we have so far (which is in the routing module), as that would be logic added to forwarders to treat that name as a keyword in a special way

negative connotation for the word "inject" -> this is the one i'm most unsure about. we can rename, but did not want the naming process to block the implementation.

@jaczhi jaczhi requested review from zjkmxy and Pesa May 4, 2025 23:44
@Pesa
Copy link
Member

Pesa commented May 5, 2025

@Pesa I believe we informally stabilized on a few conclusions for the questions brought up:
[...]

Please post these on redmine, I'm trying to keep all the protocol-level discussion in one place.

Copy link
Member

@zjkmxy zjkmxy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for my concern on chained certificate fetch.

@pulsejet
Copy link
Collaborator

pulsejet commented May 9, 2025

Pls hold off on the merge button, I'll get to this shortly

@zjkmxy zjkmxy assigned zjkmxy and pulsejet and unassigned zjkmxy May 16, 2025
@jaczhi jaczhi changed the title Prefix Injection Support in dv Prefix Insertion Support in dv Jun 2, 2025
@jaczhi
Copy link
Author

jaczhi commented Jun 2, 2025

Renamed "prefix injection" to "prefix insertion".

Copy link
Member

@zjkmxy zjkmxy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

buf[pos] = 253
binary.BigEndian.PutUint16(buf[pos+1:], uint16(534))
pos += 3
pos += uint(enc.TLNum(len(value.StapledCertificates)).EncodeInto(buf[pos:]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec requires all Certificates to be nested in the TLV-VALUE of a single StapledCertificates.
This code would emit a separate StapledCertificates TLV for each Certificate.

UseDataNameFwHint optional.Optional[bool]
// Fetch function to use for fetching certificates.
// The fetcher MUST check the store for the certificate before fetching.
Fetch optional.Optional[func(enc.Name, *InterestConfig, ExpressCallbackFunc)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fake fetch approach can result in overly complex application code.
It should be further abstracted to directly support "preloaded certificates" use cases, which include both stapled certificates in prefix insertion protocol and the certificate bundle format.
The abstraction can include the mandatory checking of the store, so that application code does not need to do so.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which include both stapled certificates in prefix insertion protocol and the certificate bundle format.

Hm I see. What would be a sufficiently general type(s) for the idea you had in mind?

Copy link
Member

@yoursunny yoursunny Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type CertSource interface {
  func(KeyLocator) (Certificate, error)
}
  • The function can obtain certificate from multiple sources: the network, the local KeyChain, the preloaded certificates from a bundle / stapled list, etc.
  • It's unnecessary to use Optional wrapper on any function or interface type, because the nil value can naturally indicate the absence of a function object.
  • The Go convention is to use a blocking function that returns the result and an error, instead of callbacks. This allows better readability, and prevents the mistake of forgetting to invoke the callback.

The library can then provide several functions for use as the above parameter:

func NewNetworkCertSource(engine) CertSource
func NewListCertSource(certs []Certificate) CertSource
func NewKeyChainCertSource(keyChain) CertSource
func NewMultiCertSource(sources ...CertSource) CertSource

To suppose both network fetching and stapled certificates:

certSource := NewMultiCertSource(
  NewListCertSource(certs),
  NewNetworkCertSource(engine),
)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoursunny how deep should this change go? Should I drill this new interface down to the Validate method in trust_config.go as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't decide for you.

# [optional] Trust schema for prefix insertion
# - If "insecure" is specified, security is disabled
# - If "deny" is specified, the prefix insertion handler will be turned off
# - Example: /absolute/path/to/schema.tlv
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A concrete example of the trust schema should be included as part of the repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants