Skip to content
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

Don't create an RRDP delta for publishers entries without content #1181

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

timbru
Copy link
Contributor

@timbru timbru commented Dec 27, 2023

No description provided.

@timbru timbru linked an issue Dec 27, 2023 that may be closed by this pull request
@timbru
Copy link
Contributor Author

timbru commented Dec 27, 2023

This should prevent empty RRDP deltas from being produced by not staging empty publication (RFC8181) deltas for the publisher and skipping them in the decision process to decide whether to produce a new RRDP delta.

As far as I can tell Krill CAs do not send empty publication deltas but issue #1180 shows that empty RRDP deltas were produced possibly because non-Krill publishers sent them.

@ximon18 ximon18 added this to In progress in 0.14.x Jan 3, 2024
@ximon18
Copy link
Member

ximon18 commented Jan 3, 2024

Notes to self:

  • Ideally I think this PR should include a test that shows that the issue occurs and is fixed.
  • Given that the RFC defines an XML schema to validate against, do the Krill tests include anywhere XML schema compliance validation and if not, should they? (though perhaps not as part of this PR)

@timbru
Copy link
Contributor Author

timbru commented Jan 3, 2024

Notes to self:

  • Ideally I think this PR should include a test that shows that the issue occurs and is fixed.

Ideally, yes. But it was not trivial to do this...

There are tests in place (under the tests directory) that have CAs publish at a Krill Server. However, Krill CAs do not send empty publication requests. Setting up an intentionally broken (or well, at least different) publishing CA is not trivial. Unit testing at the current function level is even less trivial because it would require a lot of set up and new code.

Extracting if self.staged_elements.values().any(|el| !el.0.is_empty()) { to a function could allow for some unit testing of that function with specific instances of self, but when implementing this I felt that was taking things a bit too far, as the test then becomes very tied to the implementation and become rather trivial.

Anyway... not saying any of this in protest against adding a test. Just trying to explain why there isn't one.

  • Given that the RFC defines an XML schema to validate against, do the Krill tests include anywhere XML schema compliance validation and if not, should they? (though perhaps not as part of this PR)

I am not sure, this kind of leads to formal schema validation in the XML library - which is not supported. Maybe, but it would need case by case evaluation. In this particular case it would not help as the internal state with an empty delta should be prevented. The XML is just a representation of that that state. So failing the XML generation would not help, and could even lead to irrecoverable issues (theoretically).

@ximon18
Copy link
Member

ximon18 commented Jan 3, 2024

  1. Thanks for the insights, appreciated.

  2. Re XML validation I was referring more generally to showing that the XML produced is RFC compliant, not that XML representation is the right level to test this change at.

0.14.x automation moved this from In progress to Reviewer approved Jun 11, 2024
@partim partim merged commit daad298 into main Jun 11, 2024
18 checks passed
0.14.x automation moved this from Reviewer approved to Done Jun 11, 2024
@partim partim changed the title Don't create an RRDP delta for publishers entries without content #1180 Don't create an RRDP delta for publishers entries without content Jun 11, 2024
@partim partim deleted the fix-rrdp-empty-deltas branch June 17, 2024 10:33
partim added a commit that referenced this pull request Jun 21, 2024
New

* Allow overriding the initial manifest number when initializing the TA
  signer, either by specifying `--initial_manifest_number` in the CLI or by
  including `ta_mft_nr_override: #nr` in the `ImportTa` JSON. ([#1178])
* Allow overriding the TA manifest number when signing a TA proxy request by
  specifying `--ta_mft_number_override` in the CLI. ([#1178])

Bug fixes

* Prevent empty RRDP delta lists to be produced. ([#1181])
* Correctly encode empty revocation lists in CRLs. (via [rpki-rs#295])
* Allow read access to the RIS dump while downloading a new dump.
  ([#1179])
* Don’t apply “child revoke key” command if the resource class does not
  exist. ([#1208])

Other changes

* The minimum supported Rust version is now 1.70.0. ([#1198])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

rrdp content not matching XML schema
3 participants