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.

Sign up for GitHub

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

Jump to bottom

Add withValidityPeriod to MFT and CRL builders #141

Merged
merged 6 commits into from
Jan 10, 2024
Merged

Add withValidityPeriod to MFT and CRL builders #141

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d7a2092
Add withValidityPeriod to MFT and CRL builders
lolepezy Dec 14, 2023
9572329
Update readme
lolepezy Dec 14, 2023
0490fe8
Deprecate withNextUpdateTime and withNextUpdateTime
lolepezy Dec 18, 2023
a5b9cee
Do not use deprecated method in tests unless necessary
lolepezy Jan 5, 2024
61db747
Suppress deprecation wwarnings
lolepezy Jan 10, 2024
c3d74e3
Merge branch 'main' into add-validity-period-to-mft-crl-builders
lolepezy Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ next (snapshot) release, e.g. `1.1-SNAPSHOT` after releasing `1.0`.

## 2024-xx-xx
* Add support for router certificates to the time parsing in `SignedObjectUtil`.
* Add withValidityPeriod to manifest and CRL builders

## 2023-10-31 1.36
* Access the certificate for the generic signed object parser.
Expand Down
16 changes: 16 additions & 0 deletions src/main/java/net/ripe/rpki/commons/crypto/cms/manifest/ManifestCmsBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package net.ripe.rpki.commons.crypto.cms.manifest;

import net.ripe.rpki.commons.crypto.ValidityPeriod;
import net.ripe.rpki.commons.crypto.cms.RpkiSignedObjectBuilder;
import net.ripe.rpki.commons.crypto.util.Asn1Util;
import net.ripe.rpki.commons.crypto.x509cert.X509CertificateBuilderHelper;
Expand Down Expand Up @@ -47,16 +48,31 @@ public ManifestCmsBuilder withManifestNumber(BigInteger number) {
return this;
}

/**
* @deprecated Use {@link #withValidityPeriod} instead
*/
@Deprecated
public ManifestCmsBuilder withThisUpdateTime(DateTime instant) {
this.thisUpdateTime = instant;
return this;
}

/**
* @deprecated Use {@link #withValidityPeriod} instead
*/
@Deprecated
public ManifestCmsBuilder withNextUpdateTime(DateTime instant) {
this.nextUpdateTime = instant;
return this;
}

// This is preferred since ValidityPeriod will validate the dates
public ManifestCmsBuilder withValidityPeriod(ValidityPeriod validityPeriod) {
this.thisUpdateTime = validityPeriod.getNotValidBefore();
this.nextUpdateTime = validityPeriod.getNotValidAfter();
return this;
}

public ManifestCmsBuilder withSignatureProvider(String signatureProvider) {
this.signatureProvider = signatureProvider;
return this;
Expand Down
24 changes: 19 additions & 5 deletions src/main/java/net/ripe/rpki/commons/crypto/crl/X509CrlBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package net.ripe.rpki.commons.crypto.crl;

import lombok.Getter;
import net.ripe.rpki.commons.crypto.ValidityPeriod;
import net.ripe.rpki.commons.crypto.cms.manifest.ManifestCmsBuilder;
import net.ripe.rpki.commons.crypto.crl.X509Crl.Entry;
import net.ripe.rpki.commons.crypto.util.BouncyCastleUtil;
import net.ripe.rpki.commons.crypto.x509cert.X509CertificateBuilderHelper;
Expand Down Expand Up @@ -29,7 +32,9 @@ public class X509CrlBuilder {
public static final int CRL_VERSION_2 = 2;

private X500Principal issuerDN;
@Getter
private DateTime thisUpdateTime;
@Getter
private DateTime nextUpdateTime;
private AuthorityKeyIdentifier authorityKeyIdentifier;
private CRLNumber crlNumber;
Expand All @@ -48,24 +53,33 @@ public X509CrlBuilder withIssuerDN(X500Principal issuerDN) {
return this;
}

/**
* @deprecated Use {@link #withValidityPeriod} instead
*/
@Deprecated
public X509CrlBuilder withThisUpdateTime(DateTime instant) {
this.thisUpdateTime = instant;
return this;
}

public DateTime getThisUpdateTime() {
return thisUpdateTime;
}

/**
* @deprecated Use {@link #withValidityPeriod} instead
*/
@Deprecated
public X509CrlBuilder withNextUpdateTime(DateTime instant) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can even deprecate the old ones with annotations since they are a footcannon? Not sure if we ever need to set just one.

Copy link
Member

Choose a reason for hiding this comment

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

It's also curious to think about where we can check that a validityperiod is sane, but that would be hard for any valid object. Maybe in the validation checks in the generic signed object parser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point,
search says that they are not used separately anywhere
https://github.com/search?q=org%3ARIPE-NCC%20withNextUpdateTime&type=code
So I'll mark them as deprecated

Copy link
Contributor Author

@lolepezy lolepezy Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ValidityPeriod does the check that the second date is >= the first one, so it's relatively sane. Can't come up with a good idea what is the use case of notValidBefore.isEqual(notValidAfter) though.

this.nextUpdateTime = instant;
return this;
}

public DateTime getNextUpdateTime() {
return nextUpdateTime;
// This is preferred since ValidityPeriod will validate the dates
public X509CrlBuilder withValidityPeriod(ValidityPeriod validityPeriod) {
this.thisUpdateTime = validityPeriod.getNotValidBefore();
this.nextUpdateTime = validityPeriod.getNotValidAfter();
return this;
}


/**
* CRL number must be representable in 20 octets
* https://tools.ietf.org/html/rfc5280#section-5.2.3
Expand Down
35 changes: 15 additions & 20 deletions src/test/java/net/ripe/rpki/commons/crypto/cms/manifest/ManifestCMSBuilderPropertyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.pholser.junit.quickcheck.Property;
import com.pholser.junit.quickcheck.runner.JUnitQuickcheck;
import net.ripe.rpki.commons.crypto.ValidityPeriod;
import org.joda.time.DateTime;
import org.junit.runner.RunWith;

Expand All @@ -14,27 +15,21 @@

@RunWith(JUnitQuickcheck.class)
public class ManifestCMSBuilderPropertyTest {

@Property public void buildEncodedParseCheck(
byte[] content,
BigInteger manifestNumber,
Integer validityHours
){
ManifestCmsBuilder builder = new ManifestCmsBuilder();
builder.addFile("test.crl", content);
builder.withManifestNumber(manifestNumber);
builder.withSignatureProvider(DEFAULT_SIGNATURE_PROVIDER);
builder.withCertificate(createValidManifestEECertificate(TEST_KEY_PAIR));
DateTime start = DateTime.now();
builder.withThisUpdateTime(start);
builder.withNextUpdateTime(start.plusHours(validityHours));
ManifestCms manifestCms = builder.build(TEST_KEY_PAIR.getPrivate());

ManifestCmsParser mftParser = new ManifestCmsParser();
mftParser.parse("test.mft", manifestCms.getEncoded());
assertTrue(mftParser.getManifestCms().containsFile("test.crl"));
@Property
public void buildEncodedParseCheck(byte[] content, BigInteger manifestNumber, Integer validityHours) {
ManifestCmsBuilder builder = new ManifestCmsBuilder();
builder.addFile("test.crl", content);
builder.withManifestNumber(manifestNumber);
builder.withSignatureProvider(DEFAULT_SIGNATURE_PROVIDER);
builder.withCertificate(createValidManifestEECertificate(TEST_KEY_PAIR));
DateTime start = DateTime.now();
builder.withValidityPeriod(new ValidityPeriod(start, start.plusHours(Math.abs(validityHours))));
ManifestCms manifestCms = builder.build(TEST_KEY_PAIR.getPrivate());

ManifestCmsParser mftParser = new ManifestCmsParser();
mftParser.parse("test.mft", manifestCms.getEncoded());
assertTrue(mftParser.getManifestCms().containsFile("test.crl"));
}


}

28 changes: 26 additions & 2 deletions src/test/java/net/ripe/rpki/commons/crypto/cms/manifest/ManifestCmsBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package net.ripe.rpki.commons.crypto.cms.manifest;

import net.ripe.rpki.commons.FixedDateRule;
import net.ripe.rpki.commons.crypto.ValidityPeriod;
import org.bouncycastle.util.encoders.Hex;
import org.junit.Before;
import org.junit.Rule;
Expand All @@ -26,8 +27,7 @@
@Before
public void setUp() {
subject.withManifestNumber(BigInteger.valueOf(68));
subject.withThisUpdateTime(THIS_UPDATE_TIME);
subject.withNextUpdateTime(NEXT_UPDATE_TIME);
subject.withValidityPeriod(new ValidityPeriod(THIS_UPDATE_TIME, NEXT_UPDATE_TIME));
subject.withCertificate(createValidManifestEECertificate());
subject.withSignatureProvider(DEFAULT_SIGNATURE_PROVIDER);
}
Expand Down Expand Up @@ -96,4 +96,28 @@
subject.addFile("BaR", BAR_CONTENT);
assertArrayEquals(ENCODED_MANIFEST, subject.encodeManifest());
}

@Test
lolepezy marked this conversation as resolved.
Show resolved Hide resolved
@SuppressWarnings("deprecation")
public void shouldBeEquivalentToSetDateInDifferentWays() {
var builder1 = new ManifestCmsBuilder();
builder1.withManifestNumber(BigInteger.valueOf(68));
builder1.withThisUpdateTime(THIS_UPDATE_TIME);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
ManifestCmsBuilder.withThisUpdateTime
Loading
should be avoided because it has been deprecated.
builder1.withNextUpdateTime(NEXT_UPDATE_TIME);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
ManifestCmsBuilder.withNextUpdateTime
Loading
should be avoided because it has been deprecated.
builder1.withCertificate(createValidManifestEECertificate());
builder1.withSignatureProvider(DEFAULT_SIGNATURE_PROVIDER);

var builder2 = new ManifestCmsBuilder();
builder2.withManifestNumber(BigInteger.valueOf(68));
builder2.withValidityPeriod(new ValidityPeriod(THIS_UPDATE_TIME, NEXT_UPDATE_TIME));
builder2.withNextUpdateTime(NEXT_UPDATE_TIME);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
ManifestCmsBuilder.withNextUpdateTime
Loading
should be avoided because it has been deprecated.
builder2.withCertificate(createValidManifestEECertificate());
builder2.withSignatureProvider(DEFAULT_SIGNATURE_PROVIDER);

ManifestCms m1 = builder1.build(TEST_KEY_PAIR.getPrivate());
ManifestCms m2 = builder1.build(TEST_KEY_PAIR.getPrivate());

assertEquals(m1.getThisUpdateTime(), m2.getThisUpdateTime());
assertEquals(m1.getNextUpdateTime(), m2.getNextUpdateTime());
}
}
4 changes: 2 additions & 2 deletions src/test/java/net/ripe/rpki/commons/crypto/cms/manifest/ManifestCmsParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ public void setUp() {
DateTimeUtils.setCurrentMillisFixed(THIS_UPDATE_TIME.getMillis());
ManifestCmsBuilder builder = new ManifestCmsBuilder();
builder.withCertificate(createValidManifestEECertificate()).withManifestNumber(BigInteger.valueOf(68));
builder.withThisUpdateTime(THIS_UPDATE_TIME).withNextUpdateTime(NEXT_UPDATE_TIME);
builder.withValidityPeriod(new ValidityPeriod(THIS_UPDATE_TIME, NEXT_UPDATE_TIME));
builder.addFile("foo1", FOO_CONTENT);
builder.addFile("BaR", BAR_CONTENT);
builder.withSignatureProvider(DEFAULT_SIGNATURE_PROVIDER);
Expand Down Expand Up @@ -201,7 +201,7 @@ public void shouldRejectManifestWithNonInheritEECert() {

// Use 10/8 EE cert
builder.withCertificate(createTenSlashEightResourceCertificate()).withManifestNumber(BigInteger.valueOf(68));
builder.withThisUpdateTime(THIS_UPDATE_TIME).withNextUpdateTime(NEXT_UPDATE_TIME);
builder.withValidityPeriod(new ValidityPeriod(THIS_UPDATE_TIME, NEXT_UPDATE_TIME));
builder.addFile("foo1", FOO_CONTENT);
builder.addFile("BaR", BAR_CONTENT);

Expand Down
12 changes: 8 additions & 4 deletions src/test/java/net/ripe/rpki/commons/crypto/cms/manifest/ManifestCmsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,15 +225,19 @@ public void shouldRejectWhenThisUpdateTimeIsNotBeforeNextUpdateTime() {

IpResourceSet resources = rootCertificate.getResources();

CertificateRepositoryObjectValidationContext context = new CertificateRepositoryObjectValidationContext(ROOT_CERTIFICATE_LOCATION, rootCertificate, resources, Lists.newArrayList(rootCertificate.getSubject().getName()));
CertificateRepositoryObjectValidationContext context = new CertificateRepositoryObjectValidationContext(
ROOT_CERTIFICATE_LOCATION, rootCertificate, resources, Lists.newArrayList(rootCertificate.getSubject().getName()));
ValidationResult result = ValidationResult.withLocation(ROOT_SIA_MANIFEST_RSYNC_LOCATION);

subject.validateWithCrl(ROOT_SIA_MANIFEST_RSYNC_LOCATION.toASCIIString(), context, ValidationOptions.strictValidation(), result, crl);

assertTrue(result.hasFailures());
assertEquals(
new ValidationCheck(ValidationStatus.ERROR, ValidationString.MANIFEST_THIS_UPDATE_TIME_BEFORE_NEXT_UPDATE_TIME, NEXT_UPDATE_TIME.plusSeconds(1).toString(), NEXT_UPDATE_TIME.toString()),
result.getResult(new ValidationLocation(ROOT_SIA_MANIFEST_RSYNC_LOCATION), ValidationString.MANIFEST_THIS_UPDATE_TIME_BEFORE_NEXT_UPDATE_TIME)
new ValidationCheck(ValidationStatus.ERROR,
ValidationString.MANIFEST_THIS_UPDATE_TIME_BEFORE_NEXT_UPDATE_TIME,
NEXT_UPDATE_TIME.plusSeconds(1).toString(), NEXT_UPDATE_TIME.toString()),
result.getResult(new ValidationLocation(ROOT_SIA_MANIFEST_RSYNC_LOCATION),
ValidationString.MANIFEST_THIS_UPDATE_TIME_BEFORE_NEXT_UPDATE_TIME)
);
}

Expand Down Expand Up @@ -455,7 +459,7 @@ public static ManifestCmsBuilder getRootManifestBuilder(ValidityPeriod validityP
ManifestCmsBuilder builder = new ManifestCmsBuilder();
builder.withCertificate(getManifestEEResourceCertificateBuilder().build());
builder.withManifestNumber(BigInteger.valueOf(68));
builder.withThisUpdateTime(validityPeriod.getNotValidBefore()).withNextUpdateTime(validityPeriod.getNotValidAfter());
builder.withValidityPeriod(validityPeriod);
builder.withSignatureProvider(DEFAULT_SIGNATURE_PROVIDER);
return builder;
}
Expand Down
30 changes: 30 additions & 0 deletions src/test/java/net/ripe/rpki/commons/crypto/crl/X509CrlBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package net.ripe.rpki.commons.crypto.crl;

import net.ripe.rpki.commons.crypto.ValidityPeriod;
import net.ripe.rpki.commons.crypto.cms.manifest.ManifestCms;
import net.ripe.rpki.commons.crypto.cms.manifest.ManifestCmsBuilder;
import net.ripe.rpki.commons.crypto.util.KeyPairFactoryTest;
import net.ripe.rpki.commons.crypto.util.KeyPairUtil;
import net.ripe.rpki.commons.util.UTC;
Expand All @@ -19,6 +22,8 @@
import java.security.SignatureException;
import java.security.cert.CRLException;

import static net.ripe.rpki.commons.crypto.cms.manifest.ManifestCmsParserTest.*;
import static net.ripe.rpki.commons.crypto.cms.manifest.ManifestCmsParserTest.TEST_KEY_PAIR;
import static net.ripe.rpki.commons.crypto.x509cert.X509CertificateBuilderHelper.*;
import static org.junit.Assert.*;

Expand Down Expand Up @@ -152,4 +157,29 @@
public void shouldRejectVeryBigCrlNumber() {
subject.withNumber(BigInteger.valueOf(2).pow(160));
}

@Test
@SuppressWarnings("deprecation")
public void shouldBeEquivalentToSetDateInDifferentWays() {
lolepezy marked this conversation as resolved.
Show resolved Hide resolved
var builder1 = new X509CrlBuilder();
builder1.withIssuerDN(new X500Principal("CN=ROOT"));
builder1.withThisUpdateTime(THIS_UPDATE_TIME);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
X509CrlBuilder.withThisUpdateTime
Loading
should be avoided because it has been deprecated.
builder1.withNextUpdateTime(NEXT_UPDATE_TIME);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
X509CrlBuilder.withNextUpdateTime
Loading
should be avoided because it has been deprecated.
builder1.withNumber(BigInteger.ONE);
builder1.withAuthorityKeyIdentifier(PUBLIC_KEY);
builder1.withSignatureProvider(DEFAULT_SIGNATURE_PROVIDER);

var builder2 = new X509CrlBuilder();
builder2.withIssuerDN(new X500Principal("CN=ROOT"));
builder2.withValidityPeriod(new ValidityPeriod(THIS_UPDATE_TIME, NEXT_UPDATE_TIME));
builder2.withNumber(BigInteger.ONE);
builder2.withAuthorityKeyIdentifier(PUBLIC_KEY);
builder2.withSignatureProvider(DEFAULT_SIGNATURE_PROVIDER);

var c1 = builder1.build(PRIVATE_KEY);
var c2 = builder2.build(PRIVATE_KEY);

assertEquals(c1.getThisUpdateTime(), c2.getThisUpdateTime());
lolepezy marked this conversation as resolved.
Show resolved Hide resolved
assertEquals(c1.getNextUpdateTime(), c2.getNextUpdateTime());
}
}
Loading