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

Update client_certificate.php #283

Closed
wants to merge 1 commit into from

Conversation

aftechro
Copy link
Collaborator

  • fetch expiry date

Future to do: Show issuer - atm, shows country of issue but not the provider, maybe @johnnyq or @wrongecho can have another look here

- fetch expiry date 

Future to do: Show issuer - atm, shows country of issue but not the provider, maybe @johnnyq or @wrongecho can have another look here
@wrongecho
Copy link
Collaborator

wrongecho commented Jan 15, 2022

Heyy. Just some thoughts re certificates (#286, #285, #284):

I thought I added functionality to fetch the expiry date/issuer when the certificate was initially added in an earlier PR - #230? I think it was only added for certificate add (and not edit). Was this working for you?

I would just caution against removing the expiry/issuer from the database - I think we're better off grabbing that at certificate add/edit/update and storing the value in the database, rather than parsing the certificate on the fly every time. We also probably want to support users that want to enter the issuer/expiry manually and not add the public key? Also, I think cron relies on the expiry date being in the database at the moment.

What do you think?

@aftechro
Copy link
Collaborator Author

aftechro commented Jan 15, 2022 via email

@wrongecho
Copy link
Collaborator

Hey aftechro - I'm not a dev either and am still learning as I go! A good quarter (or more) of what I commit is heavily assisted through StackOverflow haha ;)

I absolutely love the idea of being able to automatically link certificates with domains, and also your ideas of being able to get the domain details (expiry, NS, etc) as well as the certificate(s) and pull them into ITFlow automatically.

I think your logic is good! My only thoughts were just about storing the data (so we don't have to wait for the server to try and pull it each time the page loads - maybe a "refresh" button instead?) and giving people users options for manual entry (maybe the certificate is only accessible via a VPN or something). I might be wrong here as I've just skimmed the pull requests. I just wanted to give my thoughts :).. I'll let Johnny take a look and see.

@johnnyq
Copy link
Collaborator

johnnyq commented Jan 15, 2022

I agree data should not be fetched on the fly unless requested and this data needs to stay in the database to processed by cron and other scripts

On the Add/Edit Certificate Model
I think Issued by and Expire Date fields should have a fetch button appended to the right side of the textfield if a domain is filled under the domain field by hitting that button it will then fetch the data from the internet and put the data in its respecting field.
If nothing is under domain then the buttons should be inactive.

This same thing can be done with Domain Expire under domains

This would require a bit of JavaScript to work but would be the best approach

What do you guys think?

@johnnyq
Copy link
Collaborator

johnnyq commented Jan 15, 2022

closing this pull request for now as we will take an alternative approach

@johnnyq johnnyq closed this Jan 15, 2022
@aftechro
Copy link
Collaborator Author

Agree with both of you, we just need to find the best approach based on each experience. I played a bit with that code, and i was so happy that i succeeded to do such big thing in developing :d I`m sure with practice, in few months i can get better and better :D

  • my logic and of course, based on experience - certs should be pulled from domain section, and allow manual input for vpn or other domains associated with the client.
  • domain WHOIS would also be great to have with all the zone records that it`s useful for when moving ns servers or some major rollback, you would know what was there before. ITGlue has it as Revisions

image
image

and ssl

image
image

@wrongecho
Copy link
Collaborator

Opened #289 to track this.

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.

3 participants