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

Remove templates & make IAS certificate generation more robust #459

Merged
merged 2 commits into from
Jan 12, 2024

Conversation

cmickeyb
Copy link
Contributor

@cmickeyb cmickeyb commented Jan 4, 2024

Replace the template expansion that is causing periodic file corruption errors with a more resilient method for downloading the IAS certificate. This approach removes the template completely and uses a file system move to atomically update the certificate
file.

Also uses the cmake clean to remove any generated files. We were leaving extra files in the common directory tree.

This should be an addition to PR #457 to add one more level of resilience.

If you are running in HW mode inside a firewall that does not correctly proxy the IAS certificates URL, there is a new environment variable PDO_FORCE_IAS_PROXY that can be set to "true" to force use of the configured proxy (that is,
it overrides NO_PROXY).

@cmickeyb cmickeyb requested review from g2flyer and bvavala January 4, 2024 16:36
@cmickeyb cmickeyb force-pushed the mic.jan04.ias_certificates branch 2 times, most recently from 6dec534 to 0aa44e7 Compare January 4, 2024 16:51
Copy link
Contributor

@g2flyer g2flyer left a comment

Choose a reason for hiding this comment

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

Still causes ias-cert-script to invoked twice -- i.e., would still require my PR to remove that aspect ... -- but does not lead to an error anymore. Somewhat puzzled, though about that. Beforehand it seemed the cp a b for existing b caused the error but now the mv a b for existing b does not cause an error?

Comment on lines +57 to +58
# who would otherwise attempt to retrieve the certficiates
# without a proxy server
Copy link
Contributor

Choose a reason for hiding this comment

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

Text seems a bit misleading? This is really only for folks (a) inside intel with a proxy server but (b) define no_proxy generically as intel.com rather more narrowly as, e.g., .jf.intel.com,10.0.0.0/8,134.134.0.0/16,localhost,.local,127.0.0.0/8,172.16.0.0/12,192.168.0.0/16,.devtools.intel.com,.iglb.intel.com,isscorp.intel.com,.corp.intel.com,.caas.intel.com,certificates.intel.com ?

Copy link
Contributor Author

@cmickeyb cmickeyb Jan 4, 2024

Choose a reason for hiding this comment

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

its not possible with no-proxy alone to enumerate all possible hosts that might be internal. you can certainly add in some portion of the correct hosts & make it work. or you can just set this to force access through the proxy.

and while this is specifically a problem for intel employees who don't want to enumerate all possible exceptions in the no_proxy variable, those who don't have the problem will require no changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess with my ramble in the second part i hid the main point i wanted to say is that the hack is specific to intel folks in some circumstances. Hopefully, below text makes it a bit clearer.

Suggested change
# who would otherwise attempt to retrieve the certficiates
# without a proxy server
# inside intel who would otherwise attempt to retrieve the certficiates
# without a proxy server due to `no_proxy=intel.com` or alike

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess with my ramble in the second part i hid the main point i wanted to say is that the hack is specific to intel folks in some circumstances. Hopefully, below text makes it a bit clearer.

I specifically do not want to reference intel in any external code. We KNOW that its there for us. But the code is intended to be a general (if not very important under most circumstances) solution.

@cmickeyb
Copy link
Contributor Author

cmickeyb commented Jan 4, 2024

Still causes ias-cert-script to invoked twice -- i.e., would still require my PR to remove that aspect ... -- but does not lead to an error anymore. Somewhat puzzled, though about that. Beforehand it seemed the cp a b for existing b caused the error but now the mv a b for existing b does not cause an error?

I think I already said that both would be helpful (see the comments in the PR description).

Copy link
Member

@bvavala bvavala left a comment

Choose a reason for hiding this comment

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

I think we are converging towards a solution. Here a few comments:

  1. As suggested in Move trusted root cert configuration to cmake and cleanup #440 , why don't we move the fetch in the cmake through the FILE(DOWNLOAD ...) command?
    So we can remove the script -- though I think we still have to handle the dependencies as in Add missing dependencies to prevent add_custom_command race #457 .

  2. Summarizing what I understood regarding PDO_FORCE_IAS_PROXY:

  • if one does not use a proxy, then it's irrelevant.
  • if one uses a proxy and no_proxy is set appropriately, then it's irrelevant.
  • if one uses a proxy and no_proxy is not set appropriately, then it can be useful.
    Regarding the last point, it's not clear why the option should be supported (perhaps a normal fetch and, if that fails, a retry with --noproxy might make sense). Also, if this issue occurred in multiple projects, then it would be better to set no_proxy right once, rather than enabling a similar environment variable in/for each project.
  1. Why don't we remove the IAS_CERT_REQUIRED variable? (again suggested in Move trusted root cert configuration to cmake and cleanup #440 and can be done separately)
    The variable was based on the observation that the certificate is not necessary when working with SIM mode. However, it adds complexity. Instead, a better observation is: if you can clone/get the repo from github, then you can get the certificate from IAS.

@cmickeyb
Copy link
Contributor Author

cmickeyb commented Jan 5, 2024

  1. As suggested in Move trusted root cert configuration to cmake and cleanup #440 , why don't we move the fetch in the cmake through the FILE(DOWNLOAD ...) command?
    So we can remove the script -- though I think we still have to handle the dependencies as in Add missing dependencies to prevent add_custom_command race #457 .

If you recall, the builtin FILE(DOWNLOAD...) had issues with proxy configurations. There are no overrides. I think it could be made to work, but I'm not sure what the benefits would be (other than removing the script completely).

And I'm pretty sure no one actually reads what I write in the PR documentation. We all agree that #457 should be applied as well. That PR has been merged.

  1. Summarizing what I understood regarding PDO_FORCE_IAS_PROXY:
  • if one does not use a proxy, then it's irrelevant.
  • if one uses a proxy and no_proxy is set appropriately, then it's irrelevant.
  • if one uses a proxy and no_proxy is not set appropriately, then it can be useful.

Correct.

Regarding the last point, it's not clear why the option should be supported (perhaps a normal fetch and, if that fails, a retry with --noproxy might make sense). Also, if this issue occurred in multiple projects, then it would be better to set no_proxy right once, rather than enabling a similar environment variable in/for each project.

This feels like you are just moving complexity around. Are you going to attempt to come up with a fully general solution with multiple attempts for different conditions? As Michael pointed out, this is really just for intel employees. The default behavior is completely transparent to others. And its really just one line that we can take advantage of. Any attempt at arbitrary behavior is likely to excessively complicate something that really only has one limited purpose.

  1. Why don't we remove the IAS_CERT_REQUIRED variable? (again suggested in Move trusted root cert configuration to cmake and cleanup #440 and can be done separately)
    The variable was based on the observation that the certificate is not necessary when working with SIM mode. However, it adds complexity. Instead, a better observation is: if you can clone/get the repo from github, then you can get the certificate from IAS.

Agreed. Would you add a commit to this that removes that variable? I think its a relatively simple update to the cmake file. More than that... I think if we are trying to test contracts that use IAS attestations, we'll need the root certificate anyway.

@cmickeyb cmickeyb force-pushed the mic.jan04.ias_certificates branch from 0aa44e7 to 28a7828 Compare January 5, 2024 17:46
Replace the template expansion that is causing periodic file
corruption errors with a more resilient method for downloading the IAS
certificate. This approach removes the template completely and uses a
file system move to atomically update the certificate file.

Also uses the cmake clean to remove any generated files. We were
leaving extra files in the common directory tree.

Signed-off-by: Mic Bowman <[email protected]>
@cmickeyb cmickeyb force-pushed the mic.jan04.ias_certificates branch from 28a7828 to 926a084 Compare January 8, 2024 23:58
@bvavala
Copy link
Member

bvavala commented Jan 11, 2024

If you recall, the builtin FILE(DOWNLOAD...) had issues with proxy configurations. There are no overrides. I think it could be made to work, but I'm not sure what the benefits would be (other than removing the script completely).

Having one script less is pretty desirable. But I agree that the FILE(DOWNLOAD...) behind a proxy can have issues.
We can deal with this later.

And I'm pretty sure no one actually reads what I write in the PR documentation. We all agree that #457 should be applied as well. That PR has been merged.

We all did read that! I was just confirming it.

  1. Why don't we remove the IAS_CERT_REQUIRED variable? (again suggested in Move trusted root cert configuration to cmake and cleanup #440 and can be done separately)
    The variable was based on the observation that the certificate is not necessary when working with SIM mode. However, it adds complexity. Instead, a better observation is: if you can clone/get the repo from github, then you can get the certificate from IAS.

Agreed. Would you add a commit to this that removes that variable? I think its a relatively simple update to the cmake file.

It is a little more that just one line of code. I can address that in a separate PR.

More than that... I think if we are trying to test contracts that use IAS attestations, we'll need the root certificate anyway.

Totally agree -- right now the verification procedure in SIM mode simply returns "ok".

@bvavala bvavala merged commit e62be5e into hyperledger-labs:main Jan 12, 2024
4 checks passed
@cmickeyb cmickeyb deleted the mic.jan04.ias_certificates branch January 16, 2024 00:20
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