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

LTI1.1 appears to fail with z2jh helm chart version 2.0.0 #114

Closed
phish108 opened this issue Oct 1, 2022 · 23 comments
Closed

LTI1.1 appears to fail with z2jh helm chart version 2.0.0 #114

phish108 opened this issue Oct 1, 2022 · 23 comments

Comments

@phish108
Copy link

phish108 commented Oct 1, 2022

Bug description

The z2jh helm chart version 2.0.0 is incompatible with the configuration for the LTI1.1 authenticator.

Expected behaviour

The same behaviour as under 1.2.0 would be nice.

z2jh helmchart v. 1.2.0 works as documented.

Actual behaviour

The authenticator cannot find the local configuration and thus does not verify the client id. The browser gets a 401 unknown oauth_consumer_key error.

With this respect it would be helpful, if the logs should indicate which consumer_keys are configured or at least if any consumers are present. Maybe the authenticator could fail prematurely with a helpful remark, if no consumer_keys are present.

I can see that the client sends the correct consumer_key, but is still rejected. From the location of the error in validator.py it appears that no configuration is present in the validator class.

How to reproduce

Try to get the z2jh helm chart version 2.0.0 working with the documented description.

Your personal set up

My deployment runs on GKE based zero-to-jupyterhub and I use moodle as a tool consumer.

@phish108 phish108 added the bug label Oct 1, 2022
@welcome
Copy link

welcome bot commented Oct 1, 2022

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@jgwerner
Copy link
Collaborator

jgwerner commented Oct 3, 2022

@phish108 thank you for your detailed information regarding the issue with version 2.x of z2jh helm chart version 2.x! I will do my best to replicate the error this week and compare notes to provide a fix 👍

@isaacpod
Copy link

isaacpod commented Nov 4, 2022

Hello, I want to start by expressing my thanks for the awesome work being done to make JupyterHub deployable with Kubernetes/Cloud. I was wondering if there has been any findings regarding this bug. I was also running into the same error. I've deployed to AWS EKS using the "Zero to JupyterHub with Kebernetes" and tried using LTIAuthenticator with Self-hosted Canvas and LTI 1.1. I see the same error as mentioned in the issue.

@yuvipanda
Copy link
Collaborator

Can you provide the config you used? I'm looking at https://github.com/berkeley-dsep-infra/datahub/blob/7b49d64c4bffdf3624a5477015397425df06edae/deployments/data8x/secrets/staging.yaml#L9 (the values are encrypted but you can see the structure), which uses the latest z2jh and LTI works fine.

@isaacpod
Copy link

isaacpod commented Nov 4, 2022

Sure, I just removed the real key/secret value, but this is the configuration I used for the hub portion:

hub:
  config:
    # Additional documentation related to authentication and authorization available at
    # https://zero-to-jupyterhub.readthedocs.io/en/latest/administrator/authentication.html
    JupyterHub:
      authenticator_class: ltiauthenticator.LTIAuthenticator # LTI 1.1
    LTIAuthenticator:
      consumers: { "KEY": "SECRET" }
      username_key: "lis_person_contact_email_primary"

@yuvipanda
Copy link
Collaborator

@isaacpod try calling it LTI11Authenticator instead of LTIAuthenticator

@isaacpod
Copy link

isaacpod commented Nov 4, 2022

@isaacpod try calling it LTI11Authenticator instead of LTIAuthenticator

I changed it to LTI11Authenticator and it now returns a 503 Service Unavailable error. I configured it manually in our Canvas, because the url for automatic configuration also returns with 503.

@isaacpod
Copy link

isaacpod commented Nov 4, 2022

I can post the entire config file if it helps. We deployed this on AWS EKS and have an ingress setup which works fine with Dummy authenticator.

@yuvipanda
Copy link
Collaborator

@isaacpod yeah, providing logs and the full config would help.

@isaacpod
Copy link

isaacpod commented Nov 4, 2022

@isaacpod yeah, providing logs and the full config would help.

Ok, I'll get some logs in a moment, but here's the config I just updated the cluster with, I only changes "sensitive" values:

# This file can update the JupyterHub Helm chart's default configuration values.
#
# For reference see the configuration reference and default values, but make
# sure to refer to the Helm chart version of interest to you!
#
# Introduction to YAML:     https://www.youtube.com/watch?v=cdLNKUoMc6c
# Chart config reference:   https://zero-to-jupyterhub.readthedocs.io/en/stable/resources/reference.html
# Chart default values:     https://github.com/jupyterhub/zero-to-jupyterhub-k8s/blob/HEAD/jupyterhub/values.yaml
# Available chart versions: https://jupyterhub.github.io/helm-chart/
#

hub:
  config:
    # Additional documentation related to authentication and authorization available at
    # https://zero-to-jupyterhub.readthedocs.io/en/latest/administrator/authentication.html
    JupyterHub:
      authenticator_class: ltiauthenticator.LTI11Authenticator # LTI 1.1
    LTIAuthenticator:
      consumers: { "KEY": "SECRET" }
      username_key: "lis_person_contact_email_primary"

proxy:
  service:
    annotations:
      alb.ingress.kubernetes.io/healthcheck-path: /_chp_healthz
    type: NodePort

ingress:
  enabled: true
  annotations:
    kubernetes.io/ingress.class: alb
    alb.ingress.kubernetes.io/scheme: internet-facing
    alb.ingress.kubernetes.io/certificate-arn: arn:aws:acm:us-east-2:XXXXXXXXXXXX:certificate/UUID_VALUE_HERE
  ingressClassName:
  hosts:
    - OUR.DOMAINHERE.com
  pathSuffix:
  pathType: Prefix
  tls: []

@yuvipanda
Copy link
Collaborator

@isaacpod try setting LTIAuthenticator to LTI11Authenticator` as well. I think that's the config in https://github.com/berkeley-dsep-infra/datahub/blob/7b49d64c4bffdf3624a5477015397425df06edae/deployments/data8x/secrets/staging.yaml#L9

@isaacpod
Copy link

isaacpod commented Nov 4, 2022

@isaacpod try setting LTIAuthenticator to LTI11Authenticator` as well. I think that's the config in https://github.com/berkeley-dsep-infra/datahub/blob/7b49d64c4bffdf3624a5477015397425df06edae/deployments/data8x/secrets/staging.yaml#L9

Oh, ok let me look at that and I'll give it a try.

@isaacpod
Copy link

isaacpod commented Nov 4, 2022

@yuvipanda Got it working!!! I misread your previous comment. Here's the working config:

# This file can update the JupyterHub Helm chart's default configuration values.
#
# For reference see the configuration reference and default values, but make
# sure to refer to the Helm chart version of interest to you!
#
# Introduction to YAML:     https://www.youtube.com/watch?v=cdLNKUoMc6c
# Chart config reference:   https://zero-to-jupyterhub.readthedocs.io/en/stable/resources/reference.html
# Chart default values:     https://github.com/jupyterhub/zero-to-jupyterhub-k8s/blob/HEAD/jupyterhub/values.yaml
# Available chart versions: https://jupyterhub.github.io/helm-chart/
#

hub:
  config:
    # Additional documentation related to authentication and authorization available at
    # https://zero-to-jupyterhub.readthedocs.io/en/latest/administrator/authentication.html
    JupyterHub:
      authenticator_class: ltiauthenticator.LTIAuthenticator # LTI 1.1
    LTI11Authenticator:
      consumers: { "KEY": "SECRET" }
      username_key: "lis_person_contact_email_primary"

proxy:
  service:
    annotations:
      alb.ingress.kubernetes.io/healthcheck-path: /_chp_healthz
    type: NodePort

ingress:
  enabled: true
  annotations:
    kubernetes.io/ingress.class: alb
    alb.ingress.kubernetes.io/scheme: internet-facing
    alb.ingress.kubernetes.io/certificate-arn: arn:aws:acm:us-east-2:XXXXXXXXXXXX:certificate/UUID_VALUE_HERE
  ingressClassName:
  hosts:
    - OUR.DOMAINHERE.com
  pathSuffix:
  pathType: Prefix
  tls: []

Thank you so much for your help!

@yuvipanda
Copy link
Collaborator

yw! Where was the original documentatiaon you were using? We need to update that

@isaacpod
Copy link

isaacpod commented Nov 4, 2022

yw! Where was the original documentatiaon you were using? We need to update that

It's in the README.md, in this section: https://github.com/jupyterhub/ltiauthenticator#custom-configuration-with-jupyterhubs-helm-chart

@yuvipanda
Copy link
Collaborator

@isaacpod ah, so interesting - I see in #104 that that particular change was reverted. I wonder if this is because of version mismatch between latest LTIAuthenticator and what z2jh is shipping. @jgwerner would you know?

@isaacpod
Copy link

isaacpod commented Nov 4, 2022

@isaacpod ah, so interesting - I see in #104 that that particular change was reverted. I wonder if this is because of version mismatch between latest LTIAuthenticator and what z2jh is shipping. @jgwerner would you know?

If you find out anything just let me know, or if you need anything else to help. I have all my steps documented if needed.

@jgwerner
Copy link
Collaborator

jgwerner commented Nov 9, 2022

@yuvipanda @isaacpod we are testing this today with v2.x of the JupyterHub helm chart. We were able to replicate the issue with Canvas and are looking into how we could implement a documentation and/or implementation update to streamline the use with the Z2HJ config.

Thanks for posting the detailed config and bug report, very helpful ❤️

@jgwerner
Copy link
Collaborator

jgwerner commented Nov 9, 2022

@yuvipanda @consideRatio it seems that the latest version of the juypter-hub-ltiauthenticator package isn't supported.

Thus neither the LTIAuthenticator alias for the LTI11Authenticator nor the LTI13Authenticator classes are available. We reviewed the update for version 1.3 to maintain backward compatibility. Is there a specific reason why this version is being omitted?

@yuvipanda
Copy link
Collaborator

@jgwerner I see jupyterhub/zero-to-jupyterhub-k8s#2741, will send you a note privately.

@consideRatio
Copy link
Member

consideRatio commented Mar 1, 2023

Got it working!!! I misread your previous comment. Here's the working config:

The issue was that the 1.3.0 release introduced a new name for the LTI11Authenticator which wasn't available yet. With #133 we can update the package in z2jh and with that also support the new authentication class name.

@martinclaus
Copy link
Collaborator

The issue was that the 1.3.0 release introduced a new name for the LTI11Authenticator which wasn't available yet.

Shall we deprecate the use of the old name (now alias) LTIAuthenticator before merging #133?

@consideRatio
Copy link
Member

If you want to, go for it! I think yes it makes sense, but its not so important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants