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

Domains: add ssl_status field #7398

Merged
merged 5 commits into from Sep 10, 2020
Merged

Domains: add ssl_status field #7398

merged 5 commits into from Sep 10, 2020

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Aug 20, 2020

Currently this field depends on the provider.

For cloudflare:

  • initializing
  • pending_validation
  • pending_issuance
  • pending_deployment
  • active
  • pending_deletion
  • deleted

For AWS:

  • PENDING_VALIDATION
  • SUCCESS
  • ISSUED
  • INACTIVE
  • EXPIRED
  • VALIDATION_TIMED_OUT
  • REVOKED
  • FAILED

I think these should be enough for all providers:

  • valid
  • invalid
  • pending
  • unknown

Also, currently we query the status via the API each time,
having them in the DB will allow us to retry the domain validation on
domains that aren't valid yet as opposed to retry on every domain as we
do now.

We do this retry when the user hits the domain list view,
whit this we can use a task instead.

Currently this field depends on the provider.

For cloudflare:

- initializing
- pending_validation
- pending_issuance
- pending_deployment
- active
- pending_deletion
- deleted

For AWS:

- PENDING_VALIDATION
- SUCCESS
- ISSUED
- INACTIVE
- EXPIRED
- VALIDATION_TIMED_OUT
- REVOKED
- FAILED

I think these should be enough for all providers:

- valid
- invalid
- pending
- unknown

Also, currently we query the status via the API each time,
having them in the DB will allow us to retry the domain validation on
domains that aren't valid yet as opposed to retry on every domain as we
do now.

We do this retry when the user hits the domain list view,
whit this we can use a task instead.
@stsewd stsewd requested review from agjohnson, davidfischer and a team Aug 20, 2020
_('SSL certificate status'),
max_length=30,
choices=constants.SSL_STATUS_CHOICES,
default=constants.SSL_STATUS_UNKNOWN,
Copy link
Member Author

@stsewd stsewd Aug 20, 2020

Choose a reason for hiding this comment

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

We have >4K domains in .org, so this should be fast, or hope so :D

Copy link
Contributor

@davidfischer davidfischer Aug 21, 2020

Choose a reason for hiding this comment

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

It's probably fine.

@@ -1466,6 +1466,17 @@ class Domain(models.Model):
help_text=_('Number of times this domain has been hit'),
)

# This is used in readthedocsext.
Copy link
Member Author

@stsewd stsewd Aug 20, 2020

Choose a reason for hiding this comment

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

or will be used... but you get it

Copy link
Contributor

@davidfischer davidfischer left a comment

This seems useful but I have a few questions and comments:

  • If you're going to add it, shouldn't we expose it on the domain detail view (replacing this) and the domain list
  • Are you planning a data migration to get the status for all the domains?
  • Should we expose it in the admin?
  • What happens when a certificate can no longer renew? How will this get updated?

_('SSL certificate status'),
max_length=30,
choices=constants.SSL_STATUS_CHOICES,
default=constants.SSL_STATUS_UNKNOWN,
Copy link
Contributor

@davidfischer davidfischer Aug 21, 2020

Choose a reason for hiding this comment

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

It's probably fine.

@davidfischer
Copy link
Contributor

davidfischer commented Aug 21, 2020

If you're going to add it, shouldn't we expose it on the domain detail view (replacing this) and the domain list

Thinking more on this, we may not want that. The error message coming straight from the cloud provider will provide users additional details such as a CAA error.

@stsewd
Copy link
Member Author

stsewd commented Aug 24, 2020

If you're going to add it, shouldn't we expose it on the domain detail view (replacing this) and the domain list

Yeah, that will be a later step.

Thinking more on this, we may not want that. The error message coming straight from the cloud provider will provide users additional details such as a CAA error.

Not sure if that's really useful for users, I mean, those messages mostly indicate an error on the server side than the user side, right?

Are you planning a data migration to get the status for all the domains?

Yes

Should we expose it in the admin?

Probably, yes.

What happens when a certificate can no longer renew? How will this get updated?

Not sure I understood this correctly, but the path forward for this would be:

  • Add this field to the db
  • Sync the status from the cloud provider
  • Update this field every time we fetch this result from the cloud provider
  • Use a periodic task to retry on domains that don't have the valid status
  • Maybe have a periodic task (weekly or so) to update the status from domains with the valid status?

@humitos
Copy link
Member

humitos commented Sep 3, 2020

I think the code in this PR is OK. There is some more work to do before merging it I think,

  • create a data migration (probably better a management command that we can run manually after the deploy)
  • periodic task to check status for all the domains (even valid ones: looking for those that are not valid anymore)
  • periodic task to check status only for invalid domains (maybe we could send an email to users after the 3 checks, we will need an extra field to count these, I think)
  • update code/templates where we are using domain.status and we need to use the new field domain.ssl_status
  • show this field in the admin
  • add an action in the admin to perform the status check manually

I think these should be enough for all providers:

Instead of saving our own parsed value in the database, why we don't save the real value and do the mapping when we are exposing it to the user? That way we have the exact status for the domain that have extra information about the reason of the status.

@davidfischer
Copy link
Contributor

davidfischer commented Sep 3, 2020

Not sure if that's really useful for users, I mean, those messages mostly indicate an error on the server side than the user side, right?

A CAA error for example is an error the user has to fix. We can't issue the certificate until they take action.

Not sure I understood this correctly, but the path forward for this would be

Ya, we might need to take these steps. The issue is that certificates can issue correctly but then based on actions the user takes in their DNS provider or elsewhere the certificate can stop working (by not renewing).

@stsewd
Copy link
Member Author

stsewd commented Sep 8, 2020

Instead of saving our own parsed value in the database, why we don't save the real value and do the mapping when we are exposing it to the user? That way we have the exact status for the domain that have extra information about the reason of the status.

Doing that will make it hard to filter them in the db as we would need to filter with the exact status and that will also make it depend on the provider. But I guess we could delegate this to the provider, but I still prefer to have the data normalized in the db. So not sure.

@stsewd
Copy link
Member Author

stsewd commented Sep 8, 2020

Also, we can just leave the current code there as David mentioned so the user can see the actual status from the cloud provider. Doing that isn't expensive. We mostly want to have an easy way to retry on invalid domains without having to query all of them to fetch the status.

@stsewd
Copy link
Member Author

stsewd commented Sep 8, 2020

Also, this new field is going to be used by the cdn app, so the rest of the code is going to be implemented in readthedocs-ext. If we are ok with this we can merge, so I can continue with the other tasks in the other repo.

@humitos
Copy link
Member

humitos commented Sep 9, 2020

But I guess we could delegate this to the provider

I like this idea. I think we can have the best of the two worlds with this.

Also, this new field is going to be used by the cdn app, so the rest of the code is going to be implemented in readthedocs-ext

I don't want to block this work because of this, we can refactor later if needed, tho.

humitos
humitos approved these changes Sep 9, 2020
@stsewd stsewd merged commit 65f96bd into master Sep 10, 2020
2 checks passed
@stsewd stsewd deleted the add_ssl_status branch Sep 10, 2020
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

3 participants