Skip to content

Dependebility: Bumb protobuf version #617

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

Merged
merged 1 commit into from
Feb 16, 2022
Merged

Dependebility: Bumb protobuf version #617

merged 1 commit into from
Feb 16, 2022

Conversation

jdsika
Copy link
Contributor

@jdsika jdsika commented Feb 11, 2022

@pmai there is a security issue with protobuffer that created an automatic version bumb PR in osi-validation. I am wondering three things here:

  1. Why not bumb to the newest version 3.19.4?
  2. Where are the places we have defined the protobuffer version in this repository?
  3. Do we need to create a new patch version for OSI?

@jdsika jdsika added the Analysis An issue or MR that needs expert analysis to determine what to do next label Feb 11, 2022
@jdsika jdsika requested a review from stefancyliax February 11, 2022 07:52
@pmai
Copy link
Contributor

pmai commented Feb 11, 2022

Contrary to osi-validation, which used a fixed version of protobuf (for unknown reasons, not recommended anyway), we do not specify a version of protobuf, or only specify a minimum version due to required features.

Furthermore OSI is not intended to be used with untrusted sources for many other reasons anyway; for that reason I do not think we should change anything here, except maybe dropping the version in the README entirely.

We are not in a position to force the use of a version of protobuf for unrelated, and often unnecessary reasons.

For osi-validation I have changed the requirement to be a minimum version, and moved to 3.15.0 since one could argue that the validator should be more robust.

@jdsika
Copy link
Contributor Author

jdsika commented Feb 11, 2022

So, two options:

  1. remove the version and just say "protobuffer"
  2. recommend the same minimum version as in osi-validation

@pmai
Copy link
Contributor

pmai commented Feb 11, 2022

I'd only go for option 1; for osi-validation there was already a version tie, so the proper course was to update, given that osi-validation is a validator. For OSI itself I would not talk about versions beyond what we currently have: There is little attack surface in most settings, and there are prescribed versions due to the overall build processes (e.g. need for same version across subsystems), so that all users already have to deal with that themselves.

We'll see how the CCB feels about this, but that would be my opinion.

@jdsika jdsika added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Feb 11, 2022
@jdsika
Copy link
Contributor Author

jdsika commented Feb 11, 2022

Then we have the discussion and facts for a decision in the CCB. I have labeled the PR accordingly

@jdsika jdsika added this to the V3.5.0 milestone Feb 11, 2022
@kmeids
Copy link

kmeids commented Feb 16, 2022

Output CCB 16.02.22:

  1. @pmai please merge, taking into consideration not to specify any specific protobuf version in the README file.

@kmeids kmeids added ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. and removed ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Feb 16, 2022
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
@pmai pmai force-pushed the fix/dependebility branch from cd459ce to fddf460 Compare February 16, 2022 10:49
@pmai pmai merged commit 394e7b1 into master Feb 16, 2022
@pmai pmai deleted the fix/dependebility branch February 16, 2022 10:54
Copy link
Contributor

@stefancyliax stefancyliax left a comment

Choose a reason for hiding this comment

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

Approve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis An issue or MR that needs expert analysis to determine what to do next ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants