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

* suggested patch for issue 13944 #14016

Closed

Conversation

arminfuerst
Copy link
Contributor

@arminfuerst arminfuerst commented Jan 29, 2021

CLA: trivial
Fixes #13944

  • detect expiration dates after 2049-12-31
  • strip first two digits from expiration date if expiration date is after 2049-12-31
    to make compare with current date working again
    This is my first PR for openssl - sorry for probably existing mistakes...

   - detect expiration dates after 2049-12-31
   - strip first two digits from expiration date if expiration date is after 2049-12-31
     to make compare with current date working again
@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jan 29, 2021
@t8m
Copy link
Member

t8m commented Jan 29, 2021

This is unfortunately not within what is acceptable for CLA: trivial. Could you please submit an ICLA?

@arminfuerst
Copy link
Contributor Author

This is unfortunately not within what is acceptable for CLA: trivial. Could you please submit an ICLA?

The only way I found is to send a signed printout to legal@opensslfoundation.org.
(a) Is this correct?
(b) Is there a faster (pure digital?) way?

Thank you for your support!

@t8m
Copy link
Member

t8m commented Jan 29, 2021

Yes, it is correct.

IMO there is no pure digital way. @mattcaswell ?

@mattcaswell
Copy link
Member

IMO there is no pure digital way. @mattcaswell ?

We require a signed document. The signature can be a scan of your signature inserted into the document.

@t8m
Copy link
Member

t8m commented Jan 29, 2021

As for the patch - have you considered instead of trying to fix the bad y2k handling directly in the code, to actually use ASN1_TIME_set_string to parse the time from the DB and replace ASN1_UTCTIME with ASN1_TIME which should handle the y2k correctly by itself and then do the comparison of the ASN1_TIMEs. This should simplify the function much.

@arminfuerst
Copy link
Contributor Author

As for the patch - have you considered instead of trying to fix the bad y2k handling directly in the code, to actually use ASN1_TIME_set_string to parse the time from the DB and replace ASN1_UTCTIME with ASN1_TIME which should handle the y2k correctly by itself and then do the comparison of the ASN1_TIMEs. This should simplify the function much.

Thank you for your feedback. To be honest, I don't know all these data types by heart and I assumed, ASN1_TIME could cause issues with the timezone. I'll have a closer look at it when I find some spare time again.

I just sent the ICLA. Will this "automatically" fix the "cla-check" or do I have to set additional steps?

@t8m
Copy link
Member

t8m commented Jan 29, 2021

I just sent the ICLA. Will this "automatically" fix the "cla-check" or do I have to set additional steps?

No additional steps needed for that on your side.

@mattcaswell
Copy link
Member

Close/reopen to kick CLA bot

@mattcaswell mattcaswell reopened this Jan 29, 2021
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jan 29, 2021
@arminfuerst
Copy link
Contributor Author

I created an alternate suggestion using ASN1_TIME:
arminfuerst@ec00b5c
Do you want to have a look at it first or shall I start a PR directly?

@t8m
Copy link
Member

t8m commented Jan 29, 2021

With the alternate proposal I meant using ASN1_TIME_set_string() for the time loaded from the database and ASN1_TIME_compare() to compare it with the current time. I.e. dropping all the logic to handle y2k and compare the times from the apps code.

@arminfuerst
Copy link
Contributor Author

Ok, thanks for the feedback :)

@arminfuerst
Copy link
Contributor Author

I think that's what your idea was:
arminfuerst@2790fe2

@t8m
Copy link
Member

t8m commented Feb 1, 2021

Closing in favour of #14026

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.

None yet

4 participants