-
Notifications
You must be signed in to change notification settings - Fork 24
Prefix Insertion Support in dv #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
I thought we still had a couple of open questions on the protocol. Have those been resolved? |
@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 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. |
There was a problem hiding this 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.
Pls hold off on the merge button, I'll get to this shortly |
Renamed "prefix injection" to "prefix insertion". |
There was a problem hiding this 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:])) |
There was a problem hiding this comment.
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)] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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),
)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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)