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

Move DKIM keys to database #2952

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Deltachaos
Copy link

This PR solves issue #2949.

Moving the DKIM keys to database removes the requirement for shared storage in the admin.

Currently a migration path from files to database is missing. My idea is it to add this to the migration file, but i am not to deeply into the python framework here to know if this is a good idea or not. If you have some tips i happily add this feature so that this PR can get merged.

@mergify
Copy link
Contributor

mergify bot commented Sep 23, 2023

Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails, read this.

bors bot added a commit that referenced this pull request Sep 23, 2023
@bors
Copy link
Contributor

bors bot commented Sep 23, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@nextgens
Copy link
Contributor

nextgens commented Sep 24, 2023

Ah; I have code for doing this on a branch somewhere...

IMHO we can't merge it as is: we want to address the other related DKIM tickets too: #2687 #2618 #2053 #2050 #1700 maybe even #1519

We need a separate table (as the same keys could be used cross-domains -like for ARC-), we probably want additional fields (keytype, usage, generation timestamp, status, ...)

@Deltachaos
Copy link
Author

I see. But wouldn’t having the keys i the database even without changing the current behaviour that much, make a migration to the changes you mentioned easier?

@nextgens
Copy link
Contributor

I see. But wouldn’t having the keys i the database even without changing the current behaviour that much, make a migration to the changes you mentioned easier?

I don't mind incremental improvements... but the database schema should be as close to the end-state as possible. The last thing we want is to have to lost information in the process and make migration more complex as a result.

At the very least, we want a separate table with a column for the selector, a timestamp it was generated/imported on, and a way to link it to the domain it was enabled for (keeping in mind that there may be multiple DKIM keys)

@nextgens
Copy link
Contributor

Here is what I had on my branch if you need inspiration:

0001-V1.txt

class Key(Base):
    """ A Key used for DKIM or ARC """
    __tablename__ = 'key'

    name = db.Column(db.String, primary_key=True, nullable=False)
    key_type = db.Column(db.Enum(KEY_TYPE), nullable=False)
    key_size = db.Column(db.Integer, nullable=False)
    private = db.Column(db.String, unique=True, nullable=False)
    public = db.Column(db.String, unique=True, nullable=False)
    generated = db.Column(db.DateTime, nullable=True, default=datetime.utcnow())
    active = db.Column(db.DateTime, nullable=True)
    retired = db.Column(db.DateTime, nullable=True)
    use_for_dkim = db.Column(db.Boolean, nullable=True, default=False)
    use_for_arc = db.Column(db.Boolean, nullable=True, default=False)
    """ a misnamer: Domain or Alias that makes use of it """
    domains = db.relationship('Domain', secondary=key_mapping,
            lazy='dynamic', back_populates="keys")

@nextgens nextgens added the status/wip Work in progress. This will block mergify until the label is removed. label Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/wip Work in progress. This will block mergify until the label is removed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants